[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
This revision was automatically updated to reflect the committed changes.
Closed by commit rG16c7829784f0: [clangd] Check if macro is already in the
IdentifierTable before loading it (authored by qdelacru, committed by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101870/new/
https://reviews.llvm.org/D101870
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3139,6 +3139,15 @@
Kind(CompletionItemKind::Constructor;
}
+TEST(CompletionTest, NoCrashDueToMacroOrdering) {
+ EXPECT_THAT(completions(R"cpp(
+#define ECHO(X) X
+#define ECHO2(X) ECHO(X)
+int finish_preamble = EC^HO(2);)cpp")
+ .Completions,
+ UnorderedElementsAre(Labeled("ECHO(X)"), Labeled("ECHO2(X)")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1105,14 +1105,19 @@
ExternalPreprocessorSource *PreambleMacros = PP.getExternalSource();
// As we have the names of the macros, we can look up their IdentifierInfo
// and then use this to load just the macros we want.
+ const auto = PP.getIdentifierTable();
IdentifierInfoLookup *PreambleIdentifiers =
- PP.getIdentifierTable().getExternalIdentifierLookup();
+ ITable.getExternalIdentifierLookup();
+
if (!PreambleIdentifiers || !PreambleMacros)
return;
- for (const auto : Preamble.Macros.Names)
+ for (const auto : Preamble.Macros.Names) {
+if (ITable.find(MacroName.getKey()) != ITable.end())
+ continue;
if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
if (II->isOutOfDate())
PreambleMacros->updateOutOfDateIdentifier(*II);
+ }
}
// Invokes Sema code completion on a file.
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3139,6 +3139,15 @@
Kind(CompletionItemKind::Constructor;
}
+TEST(CompletionTest, NoCrashDueToMacroOrdering) {
+ EXPECT_THAT(completions(R"cpp(
+#define ECHO(X) X
+#define ECHO2(X) ECHO(X)
+int finish_preamble = EC^HO(2);)cpp")
+ .Completions,
+ UnorderedElementsAre(Labeled("ECHO(X)"), Labeled("ECHO2(X)")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1105,14 +1105,19 @@
ExternalPreprocessorSource *PreambleMacros = PP.getExternalSource();
// As we have the names of the macros, we can look up their IdentifierInfo
// and then use this to load just the macros we want.
+ const auto = PP.getIdentifierTable();
IdentifierInfoLookup *PreambleIdentifiers =
- PP.getIdentifierTable().getExternalIdentifierLookup();
+ ITable.getExternalIdentifierLookup();
+
if (!PreambleIdentifiers || !PreambleMacros)
return;
- for (const auto : Preamble.Macros.Names)
+ for (const auto : Preamble.Macros.Names) {
+if (ITable.find(MacroName.getKey()) != ITable.end())
+ continue;
if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
if (II->isOutOfDate())
PreambleMacros->updateOutOfDateIdentifier(*II);
+ }
}
// Invokes Sema code completion on a file.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
qdelacru added a comment. Yes I need help landing this, please use qdela...@cisco.com. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101870/new/ https://reviews.llvm.org/D101870 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. please provide an email address (for attribution) if i should land this for you. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101870/new/ https://reviews.llvm.org/D101870 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
qdelacru updated this revision to Diff 343141.
qdelacru added a comment.
Added suggested new test case
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101870/new/
https://reviews.llvm.org/D101870
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3139,6 +3139,15 @@
Kind(CompletionItemKind::Constructor;
}
+TEST(CompletionTest, NoCrashDueToMacroOrdering) {
+ EXPECT_THAT(completions(R"cpp(
+#define ECHO(X) X
+#define ECHO2(X) ECHO(X)
+int finish_preamble = EC^HO(2);)cpp")
+ .Completions,
+ UnorderedElementsAre(Labeled("ECHO(X)"), Labeled("ECHO2(X)")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1105,14 +1105,19 @@
ExternalPreprocessorSource *PreambleMacros = PP.getExternalSource();
// As we have the names of the macros, we can look up their IdentifierInfo
// and then use this to load just the macros we want.
+ const auto = PP.getIdentifierTable();
IdentifierInfoLookup *PreambleIdentifiers =
- PP.getIdentifierTable().getExternalIdentifierLookup();
+ ITable.getExternalIdentifierLookup();
+
if (!PreambleIdentifiers || !PreambleMacros)
return;
- for (const auto : Preamble.Macros.Names)
+ for (const auto : Preamble.Macros.Names) {
+if (ITable.find(MacroName.getKey()) != ITable.end())
+ continue;
if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
if (II->isOutOfDate())
PreambleMacros->updateOutOfDateIdentifier(*II);
+ }
}
// Invokes Sema code completion on a file.
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3139,6 +3139,15 @@
Kind(CompletionItemKind::Constructor;
}
+TEST(CompletionTest, NoCrashDueToMacroOrdering) {
+ EXPECT_THAT(completions(R"cpp(
+#define ECHO(X) X
+#define ECHO2(X) ECHO(X)
+int finish_preamble = EC^HO(2);)cpp")
+ .Completions,
+ UnorderedElementsAre(Labeled("ECHO(X)"), Labeled("ECHO2(X)")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1105,14 +1105,19 @@
ExternalPreprocessorSource *PreambleMacros = PP.getExternalSource();
// As we have the names of the macros, we can look up their IdentifierInfo
// and then use this to load just the macros we want.
+ const auto = PP.getIdentifierTable();
IdentifierInfoLookup *PreambleIdentifiers =
- PP.getIdentifierTable().getExternalIdentifierLookup();
+ ITable.getExternalIdentifierLookup();
+
if (!PreambleIdentifiers || !PreambleMacros)
return;
- for (const auto : Preamble.Macros.Names)
+ for (const auto : Preamble.Macros.Names) {
+if (ITable.find(MacroName.getKey()) != ITable.end())
+ continue;
if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
if (II->isOutOfDate())
PreambleMacros->updateOutOfDateIdentifier(*II);
+ }
}
// Invokes Sema code completion on a file.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
kadircet added a comment.
oh wow thanks! would you mind adding a test case like:
TEST(CompletionTest, NoCrashDueToMacroOrdering) {
EXPECT_THAT(completions(R"cpp(
#define ECHO(X) X
#define ECHO2(X) ECHO(X)
int finish_preamble = E^CHO(2);)cpp")
.Completions,
UnorderedElementsAre(Labeled("ECHO"), Labeled("ECHO2")));
}
into clang-tools-extra/clangd/unittest/CodeCompleteTests.cpp ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101870/new/
https://reviews.llvm.org/D101870
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D101870: [clangd] Check if macro is already in the IdentifierTable before loading it
qdelacru created this revision. qdelacru added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman. qdelacru requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Having nested macros in the C code could cause clangd to fail an assert in clang::Preprocessor::setLoadedMacroDirective() and crash. #0 0x007acd79 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:22 #1 0x007ace30 PrintStackTraceSignalHandler(void*) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1 #2 0x007aaded llvm::sys::RunSignalHandlers() /qdelacru/llvm-project/llvm/lib/Support/Signals.cpp:76:20 #3 0x007ac7c1 SignalHandler(int) /qdelacru/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1 #4 0x7f096604db20 __restore_rt (/lib64/libpthread.so.0+0x12b20) #5 0x7f0964b307ff raise (/lib64/libc.so.6+0x377ff) #6 0x7f0964b1ac35 abort (/lib64/libc.so.6+0x21c35) #7 0x7f0964b1ab09 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21b09) #8 0x7f0964b28de6 (/lib64/libc.so.6+0x2fde6) #9 0x01004d1a clang::Preprocessor::setLoadedMacroDirective(clang::IdentifierInfo*, clang::MacroDirective*, clang::MacroDirective*) /qdelacru/llvm-project/clang/lib/Lex/PPMacroExpansion.cpp:116:5 #10 0x032287b5 clang::ASTReader::resolvePendingMacro(clang::IdentifierInfo*, clang::ASTReader::PendingMacroInfo const&) /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:2219:31 #11 0x0324f130 clang::ASTReader::finishPendingActions() /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:9249:7 #12 0x0325d15e clang::ASTReader::FinishedDeserializing() /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:11575:5 #13 0x0093c64b clang::ExternalASTSource::Deserializing::~Deserializing() /qdelacru/llvm-project/clang/include/clang/AST/ExternalASTSource.h:89:5 #14 0x0324a74f clang::ASTReader::get(llvm::StringRef) /qdelacru/llvm-project/clang/lib/Serialization/ASTReader.cpp:8038:10 #15 0x011c5708 clang::clangd::(anonymous namespace)::loadMainFilePreambleMacros(clang::Preprocessor const&, clang::clangd::PreambleData const&) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1113:44 #16 0x011c5de0 clang::clangd::(anonymous namespace)::semaCodeComplete(std::unique_ptr >, clang::CodeCompleteOptions const&, clang::clangd::(anonymous namespace)::SemaCompleteInput const&, clang::clangd::IncludeStructure*) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1190:3 #17 0x011c703b clang::clangd::(anonymous namespace)::CodeCompleteFlow::run(clang::clangd::(anonymous namespace)::SemaCompleteInput const&) && /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1395:21 #18 0x011ca87a clang::clangd::codeComplete(llvm::StringRef, clang::clangd::Position, clang::clangd::PreambleData const*, clang::clangd::ParseInputs const&, clang::clangd::CodeCompleteOptions, clang::clangd::SpeculativeFuzzyFind*) /qdelacru/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1865:78 #19 0x0119c351 clang::clangd::ClangdServer::codeComplete(llvm::StringRef, clang::clangd::Position, clang::clangd::CodeCompleteOptions const&, llvm::unique_function)>)::'lambda'(llvm::Expected)::operator()(llvm::Expected) /qdelacru/llvm-project/clang-tools-extra/clangd/ClangdServer.cpp:377:61 #20 0x011a821a void llvm::detail::UniqueFunctionBase >::CallImpl)>)::'lambda'(llvm::Expected)>(void*, llvm::Expected&) /qdelacru/llvm-project/llvm/include/llvm/ADT/FunctionExtras.h:204:16 #21 0x01554403 llvm::unique_function)>::operator()(llvm::Expectedget(). Since these macro names are store in a StringSet (has StringMap underlying container), the order of the iterator is not guaranteed to be same as the order seen in the source code. When clangd is trying to resolve nested macros it sometimes attempts to load them out of order which causes a macro to be stored twice. In the example above, ECHO2 macro gets resolved first, but since it uses another macro that has not been resolved it will try to resolve/store that as well. Now there are two MacroDirectives stored in the Preprocessor, ECHO and ECHO2. When clangd tries to load the next macro, ECHO, the preprocessor fails an assert in clang::Preprocessor::setLoadedMacroDirective() because there is already a MacroDirective stored for that macro name. In this diff, I check if the macro is already inside the IdentifierTable and if it is skip it so that it is not resolved twice. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D101870 Files: clang-tools-extra/clangd/CodeComplete.cpp Index: clang-tools-extra/clangd/CodeComplete.cpp === ---
