[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, i think this LG.
Comment at:
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1697-1698
+ )cpp");
+ TU.HeaderFilename = "Foo.h";
+ auto Symbols = TU.headerSymbols();
+ EXPECT_THAT(Symbols, Not(Contains(qName("HEADER_GUARD_";
`headerSymbols` still uses `Filename` not `HeaderFilename` of the TU.
Comment at:
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1699-1702
+ 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")));
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78038/new/
https://reviews.llvm.org/D78038
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
sammccall added a comment. 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. Comment at: clang/lib/Lex/Lexer.cpp:2749 +// most useful answer is "yes, this file has a header guard". +if (!ConditionalStack.empty()) + MIOpt.ExitTopLevelConditional(); kadircet wrote: > 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. (this is obsolete as this code landed separately) Comment at: clang/lib/Serialization/ASTReader.cpp:6155 + // 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()) == kadircet wrote: > 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`? 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
sammccall updated this revision to Diff 540985.
sammccall marked an inline comment as done.
sammccall added a comment.
oops
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78038/new/
https://reviews.llvm.org/D78038
Files:
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
llvm/include/llvm/Support/OnDiskHashTable.h
Index: llvm/include/llvm/Support/OnDiskHashTable.h
===
--- llvm/include/llvm/Support/OnDiskHashTable.h
+++ llvm/include/llvm/Support/OnDiskHashTable.h
@@ -319,8 +319,8 @@
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:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1923,6 +1923,10 @@
SmallVector FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+ const auto = 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());
@@ -1963,6 +1967,14 @@
};
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.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6354,6 +6354,17 @@
// 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;
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1684,6 +1684,45 @@
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()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
sammccall updated this revision to Diff 540981.
sammccall added a comment.
rebase and narrow scope
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78038/new/
https://reviews.llvm.org/D78038
Files:
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
llvm/include/llvm/Support/OnDiskHashTable.h
Index: llvm/include/llvm/Support/OnDiskHashTable.h
===
--- llvm/include/llvm/Support/OnDiskHashTable.h
+++ llvm/include/llvm/Support/OnDiskHashTable.h
@@ -319,8 +319,8 @@
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:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1923,6 +1923,11 @@
SmallVector FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+ const auto = Context->getSourceManager();
+ unsigned MainFileUID = -1;
+ if (this->WritingAST)
+ if (const auto *Entry = SM.getFileEntryForID(SM.getMainFileID()))
+MainFileUID = Entry->getUID();
if (FilesByUID.size() > HS.header_file_size())
FilesByUID.resize(HS.header_file_size());
@@ -1963,6 +1968,14 @@
};
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.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6354,6 +6354,17 @@
// 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;
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1684,6 +1684,45 @@
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()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
nridge added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
nridge added reviewers: hokein, kadircet, VitaNuo. nridge added a comment. Let's try adding some reviewers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
voertler added a comment. We stumbled upon this issue too. What is needed to pull in this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
nridge added a comment. Herald added projects: LLVM, clang-tools-extra, All. Herald added a subscriber: llvm-commits. @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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
kadircet added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:2749 +// most useful answer is "yes, this file has a header guard". +if (!ConditionalStack.empty()) + MIOpt.ExitTopLevelConditional(); 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. Comment at: clang/lib/Serialization/ASTReader.cpp:6155 + // 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()) == 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`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78038/new/ https://reviews.llvm.org/D78038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion
sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D78038
Files:
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang/lib/Lex/Lexer.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
llvm/include/llvm/Support/OnDiskHashTable.h
Index: llvm/include/llvm/Support/OnDiskHashTable.h
===
--- llvm/include/llvm/Support/OnDiskHashTable.h
+++ llvm/include/llvm/Support/OnDiskHashTable.h
@@ -320,8 +320,8 @@
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:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1648,7 +1648,6 @@
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)
@@ -1768,6 +1767,10 @@
SmallVector FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+ const auto = 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());
@@ -1806,6 +1809,13 @@
};
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.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6147,9 +6147,19 @@
= static_cast(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;
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -2743,6 +2743,11 @@
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();
}
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1283,6 +1283,47 @@
EXPECT_THAT(Symbols, Each(IncludeHeader()));
}
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedPragmaInPreamble) {
+ // TestTU builds with a
