Clang: Fix includes in the PCH creator

We now distinguish between the the top external headers which are used for
the PCH and all external includes which are used for watching. Adding
indirect external includes can lead to errors because some are not
protected by a header guard.

Change-Id: I01576fcf1ad25e7a6e3efc252eabbd23d5c0e727
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/tools/clangpchmanagerbackend/source/collectincludesaction.h

@@ -40,10 +40,12 @@ class CollectIncludesAction final : public clang::PreprocessOnlyAction
 {
 public:
     CollectIncludesAction(FilePathIds &includeIds,
+                          FilePathIds &topIncludeIds,
                           FilePathCachingInterface &filePathCache,
                           std::vector<uint> &excludedIncludeUID,
                           std::vector<uint> &alreadyIncludedFileUIDs)
         : m_includeIds(includeIds),
+          m_topIncludeIds(topIncludeIds),
           m_filePathCache(filePathCache),
           m_excludedIncludeUID(excludedIncludeUID),
           m_alreadyIncludedFileUIDs(alreadyIncludedFileUIDs)
@@ -61,6 +63,7 @@ public:
           auto macroPreprocessorCallbacks = new CollectIncludesPreprocessorCallbacks(
                       headerSearch,
                       m_includeIds,
+                      m_topIncludeIds,
                       m_filePathCache,
                       m_excludedIncludeUID,
                       m_alreadyIncludedFileUIDs,
@@ -81,6 +84,7 @@ public:
 
 private:
     FilePathIds &m_includeIds;
+    FilePathIds &m_topIncludeIds;
     FilePathCachingInterface &m_filePathCache;
     std::vector<uint> &m_excludedIncludeUID;
     std::vector<uint> &m_alreadyIncludedFileUIDs;

src/tools/clangpchmanagerbackend/source/collectincludespreprocessorcallbacks.h

@@ -50,12 +50,14 @@ class CollectIncludesPreprocessorCallbacks final : public clang::PPCallbacks
 public:
     CollectIncludesPreprocessorCallbacks(clang::HeaderSearch &headerSearch,
                                          FilePathIds &includeIds,
+                                         FilePathIds &topIncludeIds,
                                          FilePathCachingInterface &filePathCache,
                                          const std::vector<uint> &excludedIncludeUID,
                                          std::vector<uint> &alreadyIncludedFileUIDs,
                                          clang::SourceManager &sourceManager)
         : m_headerSearch(headerSearch),
           m_includeIds(includeIds),
+          m_topIncludeIds(topIncludeIds),
           m_filePathCache(filePathCache),
           m_excludedIncludeUID(excludedIncludeUID),
           m_alreadyIncludedFileUIDs(alreadyIncludedFileUIDs),
@@ -74,10 +76,8 @@ public:
     {
         if (!m_skipInclude && file) {
             auto fileUID = file->getUID();
-            auto sourceFileID = m_sourceManager.getFileID(hashLocation);
+            auto sourceFileUID = m_sourceManager.getFileEntryForID(m_sourceManager.getFileID(hashLocation))->getUID();
-            auto sourceFileUID = m_sourceManager.getFileEntryForID(sourceFileID)->getUID();
+            if (isNotInExcludedIncludeUID(fileUID)) {
-            if (isNotInExcludedIncludeUID(fileUID) && isNotAlreadyIncluded(sourceFileUID).first) {
-
                 auto notAlreadyIncluded = isNotAlreadyIncluded(fileUID);
                 if (notAlreadyIncluded.first) {
                     m_alreadyIncludedFileUIDs.insert(notAlreadyIncluded.second, fileUID);
@@ -85,6 +85,8 @@ public:
                     if (!filePath.empty()) {
                         FilePathId includeId = m_filePathCache.filePathId(filePath);
                         m_includeIds.emplace_back(includeId);
+                        if (isInExcludedIncludeUID(sourceFileUID))
+                            m_topIncludeIds.emplace_back(includeId);
                     }
                 }
             }
@@ -132,9 +134,14 @@ public:
 
     bool isNotInExcludedIncludeUID(uint uid) const
     {
-        return !std::binary_search(m_excludedIncludeUID.begin(),
+        return !isInExcludedIncludeUID(uid);
-                                   m_excludedIncludeUID.end(),
+    }
-                                   uid);
+
+    bool isInExcludedIncludeUID(uint uid) const
+    {
+        return std::binary_search(m_excludedIncludeUID.begin(),
+                                  m_excludedIncludeUID.end(),
+                                  uid);
     }
 
     std::pair<bool, std::vector<uint>::iterator> isNotAlreadyIncluded(uint uid) const
@@ -154,6 +161,7 @@ public:
 private:
     clang::HeaderSearch &m_headerSearch;
     FilePathIds &m_includeIds;
+    FilePathIds &m_topIncludeIds;
     FilePathCachingInterface &m_filePathCache;
     const std::vector<uint> &m_excludedIncludeUID;
     std::vector<uint> &m_alreadyIncludedFileUIDs;

src/tools/clangpchmanagerbackend/source/collectincludestoolaction.h

@@ -38,9 +38,11 @@ class CollectIncludesToolAction final : public clang::tooling::FrontendActionFac
 {
 public:
     CollectIncludesToolAction(FilePathIds &includeIds,
+                              FilePathIds &topIncludeIds,
                               FilePathCachingInterface &filePathCache,
                               const Utils::PathStringVector &excludedIncludes)
         : m_includeIds(includeIds),
+          m_topIncludeIds(topIncludeIds),
           m_filePathCache(filePathCache),
           m_excludedIncludes(excludedIncludes)
     {}
@@ -63,6 +65,7 @@ public:
     clang::FrontendAction *create() override
     {
         return new CollectIncludesAction(m_includeIds,
+                                         m_topIncludeIds,
                                          m_filePathCache,
                                          m_excludedIncludeUIDs,
                                          m_alreadyIncludedFileUIDs);
@@ -74,7 +77,7 @@ public:
         fileUIDs.reserve(m_excludedIncludes.size());
 
         for (const Utils::PathString &filePath : m_excludedIncludes) {
-            const clang::FileEntry *file = fileManager.getFile({filePath.data(), filePath.size()});
+            const clang::FileEntry *file = fileManager.getFile({filePath.data(), filePath.size()}, true);
 
             if (file)
                 fileUIDs.push_back(file->getUID());
@@ -89,6 +92,7 @@ private:
     std::vector<uint> m_alreadyIncludedFileUIDs;
     std::vector<uint> m_excludedIncludeUIDs;
     FilePathIds &m_includeIds;
+    FilePathIds &m_topIncludeIds;
     FilePathCachingInterface &m_filePathCache;
     const Utils::PathStringVector &m_excludedIncludes;
 };

src/tools/clangpchmanagerbackend/source/includecollector.cpp

@@ -44,6 +44,7 @@ void IncludeCollector::collectIncludes()
 
     auto action = std::unique_ptr<CollectIncludesToolAction>(
                 new CollectIncludesToolAction(m_includeIds,
+                                              m_topIncludeIds,
                                               m_filePathCache,
                                               m_excludedIncludes));
 
@@ -76,6 +77,13 @@ FilePathIds IncludeCollector::takeIncludeIds()
     return std::move(m_includeIds);
 }
 
+FilePathIds IncludeCollector::takeTopIncludeIds()
+{
+    std::sort(m_topIncludeIds.begin(), m_topIncludeIds.end());
+
+    return std::move(m_topIncludeIds);
+}
+
 
 
 } // namespace ClangBackEnd

src/tools/clangpchmanagerbackend/source/includecollector.h

@@ -41,10 +41,12 @@ public:
     void setExcludedIncludes(Utils::PathStringVector &&excludedIncludes);
 
     FilePathIds takeIncludeIds();
+    FilePathIds takeTopIncludeIds();
 
 private:
     Utils::PathStringVector m_excludedIncludes;
     FilePathIds m_includeIds;
+    FilePathIds m_topIncludeIds;
     Utils::SmallStringVector m_directories;
     FilePathCachingInterface &m_filePathCache;
 };

src/tools/clangpchmanagerbackend/source/pchcreator.cpp

@@ -467,7 +467,7 @@ Utils::PathStringVector PchCreator::generateProjectPartHeaderAndSourcePaths(
     return includeAndSources;
 }
 
-FilePathIds PchCreator::generateProjectPartPchIncludes(
+std::pair<FilePathIds,FilePathIds> PchCreator::generateProjectPartPchIncludes(
         const V2::ProjectPartContainer &projectPart) const
 {
     Utils::SmallString jointedFileContent = generateProjectPartHeaderAndSourcesContent(projectPart);
@@ -492,7 +492,7 @@ FilePathIds PchCreator::generateProjectPartPchIncludes(
 
     jointFile->remove();
 
-    return  collector.takeIncludeIds();
+    return  {collector.takeIncludeIds(), collector.takeTopIncludeIds()};
 }
 
 Utils::SmallString PchCreator::generateProjectPathPchHeaderFilePath(
@@ -547,8 +547,10 @@ Utils::SmallStringVector PchCreator::generateProjectPartClangCompilerArguments(
 IdPaths PchCreator::generateProjectPartPch(const V2::ProjectPartContainer &projectPart)
 {
     long long lastModified = QDateTime::currentSecsSinceEpoch();
-    auto includes = generateProjectPartPchIncludes(projectPart);
+    FilePathIds allExternalIncludes;
-    auto content = generatePchIncludeFileContent(includes);
+    FilePathIds topExternalIncludes;
+    std::tie(allExternalIncludes, topExternalIncludes) = generateProjectPartPchIncludes(projectPart);
+    auto content = generatePchIncludeFileContent(topExternalIncludes);
     auto pchIncludeFilePath = generateProjectPathPchHeaderFilePath(projectPart);
     auto pchFilePath = generateProjectPartPchFilePath(projectPart);
     generateFileWithContent(pchIncludeFilePath, content);
@@ -556,7 +558,7 @@ IdPaths PchCreator::generateProjectPartPch(const V2::ProjectPartContainer &proje
     generatePch(generateProjectPartClangCompilerArguments(projectPart),
                 {projectPart.projectPartId().clone(), std::move(pchFilePath), lastModified});
 
-    return {projectPart.projectPartId().clone(), std::move(includes)};
+    return {projectPart.projectPartId().clone(), std::move(allExternalIncludes)};
 }
 
 void PchCreator::generatePchs()

src/tools/clangpchmanagerbackend/source/pchcreator.h

@@ -97,7 +97,7 @@ unittest_public:
             const V2::ProjectPartContainer &projectPart) const;
     Utils::PathStringVector generateProjectPartHeaderAndSourcePaths(
             const V2::ProjectPartContainer &projectPart) const;
-    FilePathIds generateProjectPartPchIncludes(
+    std::pair<FilePathIds,FilePathIds> generateProjectPartPchIncludes(
             const V2::ProjectPartContainer &projectPart) const;
     Utils::SmallString generateProjectPathPchHeaderFilePath(
             const V2::ProjectPartContainer &projectPart) const;

tests/unit/unittest/data/includecollector_indirect_external.h

@@ -1 +1,3 @@
 #pragma once
+
+#include "includecollector_indirect_external2.h"

tests/unit/unittest/data/includecollector_indirect_external2.h

@@ -0,0 +1,2 @@
+#pragma once
+

tests/unit/unittest/includecollector-test.cpp

@@ -47,8 +47,21 @@ namespace {
 class IncludeCollector : public ::testing::Test
 {
 protected:
-    void SetUp();
+    void SetUp()
-    FilePathId id(const Utils::SmallStringView &path);
+    {
+        collector.addFile(TESTDATA_DIR, "includecollector_main.cpp", "", {"cc", "includecollector_main.cpp"});
+        collector.addFile(TESTDATA_DIR, "includecollector_main2.cpp", "", {"cc", "includecollector_main2.cpp"});
+
+        collector.addUnsavedFiles({{{TESTDATA_DIR, "includecollector_generated_file.h"}, "#pragma once", {}}});
+
+        collector.setExcludedIncludes(excludePaths.clone());
+        emptyCollector.setExcludedIncludes(excludePaths.clone());
+    }
+
+    FilePathId id(const Utils::SmallStringView &path)
+    {
+        return filePathCache.filePathId(FilePathView{path});
+    }
 
 protected:
     Sqlite::Database database{":memory:", Sqlite::JournalMode::Memory};
@@ -56,8 +69,8 @@ protected:
     ClangBackEnd::FilePathCaching filePathCache{database};
     ClangBackEnd::IncludeCollector collector{filePathCache};
     ClangBackEnd::IncludeCollector emptyCollector{filePathCache};
-    Utils::PathStringVector excludePaths = {TESTDATA_DIR "/includecollector_main.h",
+    Utils::PathStringVector excludePaths = {TESTDATA_DIR "/includecollector_main.cpp",
-                                            TESTDATA_DIR "/includecollector_main2.h",
+                                            TESTDATA_DIR "/includecollector_main2.cpp",
                                             TESTDATA_DIR "/includecollector_header1.h",
                                             TESTDATA_DIR "/includecollector_header2.h",
                                             TESTDATA_DIR "/includecollector_generated_file.h"};
@@ -69,7 +82,9 @@ TEST_F(IncludeCollector, IncludesExternalHeader)
 
     ASSERT_THAT(collector.takeIncludeIds(),
                 AllOf(Contains(id(TESTDATA_DIR "/includecollector_external1.h")),
-                      Contains(id(TESTDATA_DIR "/includecollector_external2.h"))));
+                      Contains(id(TESTDATA_DIR "/includecollector_external2.h")),
+                      Contains(id(TESTDATA_DIR "/includecollector_indirect_external.h")),
+                      Contains(id(TESTDATA_DIR "/includecollector_indirect_external2.h"))));
 }
 
 TEST_F(IncludeCollector, DoesNotIncludesInternalHeader)
@@ -86,7 +101,9 @@ TEST_F(IncludeCollector, NoDuplicate)
     ASSERT_THAT(collector.takeIncludeIds(),
                 UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h"),
                                      id(TESTDATA_DIR "/includecollector_external2.h"),
-                                     id(TESTDATA_DIR "/includecollector_external3.h")));
+                                     id(TESTDATA_DIR "/includecollector_external3.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external2.h")));
 }
 
 TEST_F(IncludeCollector, IncludesAreSorted)
@@ -94,7 +111,7 @@ TEST_F(IncludeCollector, IncludesAreSorted)
     collector.collectIncludes();
 
     ASSERT_THAT(collector.takeIncludeIds(),
-                SizeIs(3));
+                SizeIs(5));
 }
 
 TEST_F(IncludeCollector, If)
@@ -116,7 +133,9 @@ TEST_F(IncludeCollector, LocalPath)
     ASSERT_THAT(emptyCollector.takeIncludeIds(),
                 UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h"),
                                      id(TESTDATA_DIR "/includecollector_external2.h"),
-                                     id(TESTDATA_DIR "/includecollector_external3.h")));
+                                     id(TESTDATA_DIR "/includecollector_external3.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external2.h")));
 }
 
 TEST_F(IncludeCollector, IgnoreMissingFile)
@@ -126,23 +145,53 @@ TEST_F(IncludeCollector, IgnoreMissingFile)
     emptyCollector.collectIncludes();
 
     ASSERT_THAT(emptyCollector.takeIncludeIds(),
+                UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external.h"),
+                                     id(TESTDATA_DIR "/includecollector_indirect_external2.h")));
+}
+
+TEST_F(IncludeCollector, IncludesOnlyTopExternalHeader)
+{
+    collector.collectIncludes();
+
+    ASSERT_THAT(collector.takeTopIncludeIds(),
+                UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h"),
+                                     id(TESTDATA_DIR "/includecollector_external2.h"),
+                                     id(TESTDATA_DIR "/includecollector_external3.h")));
+}
+
+TEST_F(IncludeCollector, TopIncludeInIfMacro)
+{
+    emptyCollector.addFile(TESTDATA_DIR, "includecollector_if.cpp", "", {"cc", "includecollector_if.cpp"});
+    emptyCollector.setExcludedIncludes({"includecollector_if.cpp"});
+
+    emptyCollector.collectIncludes();
+
+    ASSERT_THAT(emptyCollector.takeTopIncludeIds(),
+                ElementsAre(id(TESTDATA_DIR "/includecollector_true.h")));
+}
+
+TEST_F(IncludeCollector, TopIncludeWithLocalPath)
+{
+    emptyCollector.addFile(TESTDATA_DIR, "includecollector_main.cpp", "", {"cc", "includecollector_main.cpp"});
+
+    emptyCollector.collectIncludes();
+
+    ASSERT_THAT(emptyCollector.takeTopIncludeIds(),
+                UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h"),
+                                     id(TESTDATA_DIR "/includecollector_external2.h"),
+                                     id(TESTDATA_DIR "/includecollector_external3.h")));
+}
+
+TEST_F(IncludeCollector, TopIncludesIgnoreMissingFile)
+{
+    emptyCollector.addFile(TESTDATA_DIR, "includecollector_missingfile.cpp", "", {"cc", "includecollector_missingfile.cpp"});
+    emptyCollector.setExcludedIncludes({"includecollector_missingfile.cpp"});
+
+    emptyCollector.collectIncludes();
+
+    ASSERT_THAT(emptyCollector.takeTopIncludeIds(),
                 UnorderedElementsAre(id(TESTDATA_DIR "/includecollector_external1.h")));
 }
 
-void IncludeCollector::SetUp()
-{
-    collector.addFile(TESTDATA_DIR, "includecollector_main.cpp", "", {"cc", "includecollector_main.cpp"});
-    collector.addFile(TESTDATA_DIR, "includecollector_main2.cpp", "", {"cc", "includecollector_main2.cpp"});
-
-    collector.addUnsavedFiles({{{TESTDATA_DIR, "includecollector_generated_file.h"}, "#pragma once", {}}});
-
-    collector.setExcludedIncludes(excludePaths.clone());
-    emptyCollector.setExcludedIncludes(excludePaths.clone());
-}
-
-FilePathId IncludeCollector::id(const Utils::SmallStringView &path)
-{
-    return filePathCache.filePathId(FilePathView{path});
-}
-
 }

tests/unit/unittest/pchcreator-test.cpp

@@ -47,6 +47,7 @@ using ClangBackEnd::ProjectPartPch;
 using ClangBackEnd::V2::ProjectPartContainer;
 using ClangBackEnd::V2::FileContainer;
 using ClangBackEnd::FilePath;
+using ClangBackEnd::FilePathIds;
 using ClangBackEnd::FilePathView;
 
 using Utils::PathString;
@@ -215,19 +216,27 @@ TEST_F(PchCreator, CreateProjectPartHeaderAndSources)
 
 TEST_F(PchCreatorSlowTest, CreateProjectPartPchIncludes)
 {
+    using IncludePair = decltype(creator.generateProjectPartPchIncludes(projectPart1));
+
     auto includeIds = creator.generateProjectPartPchIncludes(projectPart1);
 
     ASSERT_THAT(includeIds,
-                AllOf(Contains(id(TESTDATA_DIR "/includecollector_external1.h")),
+                AllOf(
-                      Contains(id(TESTDATA_DIR "/includecollector_external2.h")),
+                    Field(&IncludePair::first,
-                      Contains(id(TESTDATA_DIR "/includecollector_header2.h"))));
+                          AllOf(Contains(id(TESTDATA_DIR "/includecollector_external1.h")),
+                                Contains(id(TESTDATA_DIR "/includecollector_external2.h")),
+                                Contains(id(TESTDATA_DIR "/includecollector_header2.h")))),
+                    Field(&IncludePair::second,
+                          AllOf(Contains(id(TESTDATA_DIR "/includecollector_external1.h")),
+                                Contains(id(TESTDATA_DIR "/includecollector_external2.h"))))));
 }
 
 TEST_F(PchCreatorSlowTest, CreateProjectPartPchFileContent)
 {
-    auto includes = creator.generateProjectPartPchIncludes(projectPart1);
+    FilePathIds topExternalIncludes;
+    std::tie(std::ignore, topExternalIncludes) = creator.generateProjectPartPchIncludes(projectPart1);
 
-    auto content = creator.generatePchIncludeFileContent(includes);
+    auto content = creator.generatePchIncludeFileContent(topExternalIncludes);
 
     ASSERT_THAT(std::string(content),
                 AllOf(HasSubstr("#include \"" TESTDATA_DIR "/includecollector_header2.h\"\n"),
@@ -237,8 +246,9 @@ TEST_F(PchCreatorSlowTest, CreateProjectPartPchFileContent)
 
 TEST_F(PchCreatorSlowTest, CreateProjectPartPchIncludeFile)
 {
-    auto includeIds = creator.generateProjectPartPchIncludes(projectPart1);
+    FilePathIds topExternalIncludes;
-    auto content = creator.generatePchIncludeFileContent(includeIds);
+    std::tie(std::ignore, topExternalIncludes) = creator.generateProjectPartPchIncludes(projectPart1);
+    auto content = creator.generatePchIncludeFileContent(topExternalIncludes);
     auto pchIncludeFilePath = creator.generateProjectPathPchHeaderFilePath(projectPart1);
     auto file = creator.generateFileWithContent(pchIncludeFilePath, content);
     file->open(QIODevice::ReadOnly);