[clangd] WIP: fix several bugs relating to include insertion

[sammccall]

Summary

This should probably be untangled into 3 separate patches, but
looking for feedback first on whether we want to do this at all.

a) store include-insertion information for main-file symbols that are

not eligible for code completion indexing.

b) consider an #ifndef guard that's dangling at the end of the preamble

to be a well-formed header guard (otherwise we never will)

c) load HeaderFileInfo for the preamble main-file even if the file-size

mismatches

[kadircet]

clang/lib/Lex/Lexer.cpp

2749

I think we should put this behind a PPOpt to make sure we don't regress the rest of the world, as I fear this part of the code might not be well tested.

clang/lib/Serialization/ASTReader.cpp

6155

do we have other (apart from isFileMultiIncludeGuarded) things that depend on HFI for main file being correctly deserialized?

If it is only the include guard, maybe we can store that info in control block and later on restore it when reading HFI for main file in HeaderSearch::getExistingFileInfo?

[nridge]

@sammccall should we get this patch landed? The issue of #ifndef-guarded header files that include each other recursively is still tripping up some users.

[voertler]

We stumbled upon this issue too. What is needed to pull in this patch?

[nridge]

Let's try adding some reviewers

[nridge]

Also adding links to the open issues this fixes:

• https://github.com/clangd/clangd/issues/333
• https://github.com/clangd/clangd/issues/337

Note the second issue is not specific to include insertion; it's a false positive diagnostic, which I think makes it a more severe bug than include insertion misbehaving. That issue likely has some duplicates lying around (at least I remember it coming up multiple times) though I can't find them at the moment.

[sammccall]

rebase and narrow scope

[sammccall]

oops

[sammccall]

OK, time to revive this patch, sorry for letting it die.

Meanwhile, the lexer change has landed separately, so that's gone.

I've removed the gratuitous clangd indexing changes in order to focus this on the serialization hacks.

clang/lib/Lex/Lexer.cpp

2749

(this is obsolete as this code landed separately)

clang/lib/Serialization/ASTReader.cpp

6155

The real answer here is "I have no idea". I definitely don't have bug reports saying other things are broken.
HFI looks like it affects module semantics, so seems like it could be relevant when the main file is part of a module. But today we don't have modules support...

Really, I'd just rather make the existing mechanism work than add a second one.

[kadircet]

thanks, i think this LG.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

1296–1297

headerSymbols still uses Filename not HeaderFilename of the TU.

1298–1301

i think we don't want to just check for names of the symbols, but also want to make sure they got proper include headers assigned?

clang-tools-extra/clangd/index/SymbolCollector.cpp

const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   // We use the primary template, as clang does during code completion.
   CodeCompletionResult SymbolCompletion(&getTemplateOrThis(ND), 0);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
       *ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
       *CompletionTUInfo,
       /*IncludeBriefComments*/ false);
-  std::string Documentation =
+  std::string Documentation;
+  if (Opts.StoreAllDocumentation ||
+      S.Flags & Symbol::IndexedForCodeCompletion) {
+    Documentation =
         formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
                                                 /*CommentsFromHeaders=*/true));
-  if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
-    if (Opts.StoreAllDocumentation)
-      S.Documentation = Documentation;
-    Symbols.insert(S);
-    return Symbols.find(S.ID);
   }
-  S.Documentation = Documentation;
   std::string Signature;
   std::string SnippetSuffix;
+  if (S.Flags & Symbol::IndexedForCodeCompletion) {
     getSignature(*CCS, &Signature, &SnippetSuffix);
     S.Signature = Signature;
     S.CompletionSnippetSuffix = SnippetSuffix;
     std::string ReturnType = getReturnType(*CCS);
     S.ReturnType = ReturnType;
 
-  llvm::Optional<OpaqueType> TypeStorage;
-  if (S.Flags & Symbol::IndexedForCodeCompletion) {
-    TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);
-    if (TypeStorage)
+    if (auto TypeStorage =
+            OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion))
       S.Type = TypeStorage->raw();
   }
 
   Symbols.insert(S);
   setIncludeLocation(S, ND.getLocation());
   return Symbols.find(S.ID);
 }
 

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

runSymbolCollector(R"cpp(
 
     #endif // Header guard is recognized here.
   )cpp",
                      "");
   EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_"))));
   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")));
+  EXPECT_THAT(Symbols, Each(IncludeHeader()));
+}
+
+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")));
+  EXPECT_THAT(Symbols, Each(IncludeHeader()));
+}
+
 TEST_F(SymbolCollectorTest, NonModularHeader) {
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
 
   // Files missing include guards aren't eligible for insertion.
   TU.ImplicitHeaderGuard = false;
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader())));
 

clang/lib/Lex/Lexer.cpp

if (isLexingRawMode()) {
     Result.startToken();
     BufferPtr = BufferEnd;
     FormTokenWithChars(Result, BufferEnd, tok::eof);
     return true;
   }
 
   if (PP->isRecordingPreamble() && PP->isInPrimaryFile()) {
     PP->setRecordedPreambleConditionalStack(ConditionalStack);
+    // If the preamble cuts off the end of a header guard, consider it guarded.
+    // The guard is valid for the preamble content itself, and for tools the
+    // most useful answer is "yes, this file has a header guard".
+    if (!ConditionalStack.empty())
+      MIOpt.ExitTopLevelConditional();
     ConditionalStack.clear();
   }
 
   // Issue diagnostics for unterminated #if and missing newline.
 
   // If we are in a #if directive, emit an error.
   while (!ConditionalStack.empty()) {
     if (PP->getCodeCompletionFileLoc() != FileLoc)

clang/lib/Serialization/ASTReader.cpp

namespace {
   public:
     explicit HeaderFileInfoVisitor(const FileEntry *FE) : FE(FE) {}
 
     bool operator()(ModuleFile &M) {
       HeaderFileInfoLookupTable *Table
         = static_cast<HeaderFileInfoLookupTable *>(M.HeaderFileInfoTable);
       if (!Table)
         return false;
-
       // 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;
 
       HFI = *Pos;
       return true;
     }
 
     Optional<HeaderFileInfo> getHeaderFileInfo() const { return HFI; }

clang/lib/Serialization/ASTWriter.cpp

public:
     }
 
     void EmitData(raw_ostream &Out, key_type_ref key,
                   data_type_ref Data, unsigned DataLen) {
       using namespace llvm::support;
 
       endian::Writer LE(Out, little);
       uint64_t Start = Out.tell(); (void)Start;
-
       unsigned char Flags = (Data.HFI.isImport << 5)
                           | (Data.HFI.isPragmaOnce << 4)
                           | (Data.HFI.DirInfo << 1)
                           | Data.HFI.IndexHeaderMapHeader;
       LE.write<uint8_t>(Flags);
       LE.write<uint16_t>(Data.HFI.NumIncludes);
 
       if (!Data.HFI.ControllingMacro)
while (!Worklist.empty()) {
       }
 
       Worklist.append(M->submodule_begin(), M->submodule_end());
     }
   }
 
   SmallVector<const FileEntry *, 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());
 
   for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
     const FileEntry *File = FilesByUID[UID];
     if (!File)
       continue;
for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
     HeaderFileInfoTrait::key_type Key = {
       Filename, File->getSize(), getTimestampForOutput(File)
     };
     HeaderFileInfoTrait::data_type Data = {
       *HFI, HS.getModuleMap().findAllModulesForHeader(File), {}
     };
     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 (File->getUID() == MainFileUID) {
+      Key.Size = 0;
+      Generator.insert(Key, Data, GeneratorTrait);
+      ++NumHeaderSearchEntries;
+    }
   }
 
   // Create the on-disk hash table in a buffer.
   SmallString<4096> TableData;
   uint32_t BucketOffset;
   {
     using namespace llvm::support;
 

llvm/include/llvm/Support/OnDiskHashTable.h

public:
   offset_type getNumEntries() const { return NumEntries; }
   const unsigned char *getBase() const { return Base; }
   const unsigned char *getBuckets() const { return Buckets; }
 
   bool isEmpty() const { return NumEntries == 0; }
 
   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:
     iterator() : Key(), Data(nullptr), Len(0), InfoObj(nullptr) {}
     iterator(const internal_key_type K, const unsigned char *D, offset_type L,
              Info *InfoObj)
         : Key(K), Data(D), Len(L), InfoObj(InfoObj) {}