[clang] [clang-tools-extra] [llvm] [clangd] Avoid bogus error about recursive inclusion (PR #95200)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Nathan Ridge (HighCommander4)

<details>
<summary>Changes</summary>

Fixes https://github.com/clangd/clangd/issues/337

---
Full diff: https://github.com/llvm/llvm-project/pull/95200.diff


4 Files Affected:

- (modified) clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp (+39) 
- (modified) clang/lib/Serialization/ASTReader.cpp (+11) 
- (modified) clang/lib/Serialization/ASTWriter.cpp (+12) 
- (modified) llvm/include/llvm/Support/OnDiskHashTable.h (+2-2) 


``````````diff
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 1e7a30e69fabe..75c31f88d0fed 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1719,6 +1719,45 @@ TEST_F(SymbolCollectorTest, HeaderGuardDetected) {
   EXPECT_THAT(Symbols, Each(includeHeader()));
 }
 
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedPragmaInPreamble) {
+  // TestTU builds with a preamble.
+  auto TU = TestTU::withCode(R"cpp(
+    #pragma once
+
+    // Symbols are seen before the header guard is complete.
+    #define MACRO
+    int decl();
+    #define MACRO2
+  )cpp");
+  TU.HeaderFilename = "Foo.h";
+  auto Symbols = TU.headerSymbols();
+  EXPECT_THAT(Symbols, Not(Contains(qName("HEADER_GUARD_"))));
+  EXPECT_THAT(Symbols, Contains(qName("MACRO")));
+  EXPECT_THAT(Symbols, Contains(qName("MACRO2")));
+  EXPECT_THAT(Symbols, Contains(qName("decl")));
+}
+
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedIfdefInPreamble) {
+  // TestTU builds with a preamble.
+  auto TU = TestTU::withCode(R"cpp(
+    #ifndef HEADER_GUARD_
+    #define HEADER_GUARD_
+
+    // Symbols are seen before the header guard is complete.
+    #define MACRO
+    int decl();
+    #define MACRO2
+
+    #endif // Header guard is recognized here.
+  )cpp");
+  TU.HeaderFilename = "Foo.h";
+  auto Symbols = TU.headerSymbols();
+  EXPECT_THAT(Symbols, Not(Contains(qName("HEADER_GUARD_"))));
+  EXPECT_THAT(Symbols, Contains(qName("MACRO")));
+  EXPECT_THAT(Symbols, Contains(qName("MACRO2")));
+  EXPECT_THAT(Symbols, Contains(qName("decl")));
+}
+
 TEST_F(SymbolCollectorTest, NonModularHeader) {
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(includeHeader()));
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 44e23919ea18e..36d0e499e72b8 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -6518,6 +6518,17 @@ namespace {
 
       // Look in the on-disk hash table for an entry for this file name.
       HeaderFileInfoLookupTable::iterator Pos = Table->find(FE);
+      // Preambles may be reused with different main-file content.
+      // A second entry with size zero is stored for the main-file, try that.
+      // To avoid doing this on every miss, require the bare filename to match.
+      if (Pos == Table->end() && M.Kind == clang::serialization::MK_Preamble &&
+          llvm::sys::path::filename(FE.getName()) ==
+              llvm::sys::path::filename(M.OriginalSourceFileName)) {
+        auto InternalKey = Table->getInfoObj().GetInternalKey(FE);
+        InternalKey.Size = 0;
+        Pos = Table->find_hashed(InternalKey,
+                                 Table->getInfoObj().ComputeHash(InternalKey));
+      }
       if (Pos == Table->end())
         return false;
 
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 8a4b36207c473..0a9093e0d189b 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2044,6 +2044,10 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
 
   SmallVector<OptionalFileEntryRef, 16> FilesByUID;
   HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  const auto &SM = Context->getSourceManager();
+  unsigned MainFileUID = -1;
+  if (const auto *Entry = SM.getFileEntryForID(SM.getMainFileID()))
+    MainFileUID = Entry->getUID();
 
   if (FilesByUID.size() > HS.header_file_size())
     FilesByUID.resize(HS.header_file_size());
@@ -2082,6 +2086,14 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {
     };
     Generator.insert(Key, Data, GeneratorTrait);
     ++NumHeaderSearchEntries;
+    // We may reuse a preamble even if the rest of the file is different, so
+    // allow looking up info for the main file with a zero size.
+    if (this->getASTContext().getLangOpts().CompilingPCH &&
+        File->getUID() == MainFileUID) {
+      Key.Size = 0;
+      Generator.insert(Key, Data, GeneratorTrait);
+      ++NumHeaderSearchEntries;
+    }
   }
 
   // Create the on-disk hash table in a buffer.
diff --git a/llvm/include/llvm/Support/OnDiskHashTable.h b/llvm/include/llvm/Support/OnDiskHashTable.h
index f6b4055e74de7..561208aa9a679 100644
--- a/llvm/include/llvm/Support/OnDiskHashTable.h
+++ b/llvm/include/llvm/Support/OnDiskHashTable.h
@@ -321,8 +321,8 @@ template <typename Info> class OnDiskChainedHashTable {
 
   class iterator {
     internal_key_type Key;
-    const unsigned char *const Data;
-    const offset_type Len;
+    const unsigned char *Data;
+    offset_type Len;
     Info *InfoObj;
 
   public:

``````````

</details>


https://github.com/llvm/llvm-project/pull/95200