Skip to content

Conversation

@ArcsinX
Copy link
Contributor

@ArcsinX ArcsinX commented Aug 21, 2025

Reland #153756

LLVM_INSTANTIATE_REGISTRY(..) and FeatureModuleRegistry::entries() were in different translation units, that rises a warning when compiling with clang and leads to compilation failure if -Werror flag is set.
Instead of iterating directly in the main function, a static method FeatureModuleSet::fromRegistry() was added and it is called in the main function.

This patch adds feature modules registry, which allows to discover registered feature modules and add them into the clangd's feature module set
@llvmbot
Copy link
Member

llvmbot commented Aug 21, 2025

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

@llvm/pr-subscribers-clangd

Author: Aleksandr Platonov (ArcsinX)

Changes

Reland #153756

LLVM_INSTANTIATE_REGISTRY(..) and FeatureModuleRegistry::entries() were in different translation units, that rises a warning when compiling with clang and leads to compilation failure if -Werror flag is set.
Instead of iterating directly in the main function, a static method FeatureModuleSet::fromRegistry() was added and it is called in the main function.


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

3 Files Affected:

  • (modified) clang-tools-extra/clangd/FeatureModule.cpp (+15)
  • (modified) clang-tools-extra/clangd/FeatureModule.h (+14-3)
  • (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+5)
diff --git a/clang-tools-extra/clangd/FeatureModule.cpp b/clang-tools-extra/clangd/FeatureModule.cpp
index 872cea1443789..38461e3db4e6b 100644
--- a/clang-tools-extra/clangd/FeatureModule.cpp
+++ b/clang-tools-extra/clangd/FeatureModule.cpp
@@ -22,6 +22,10 @@ FeatureModule::Facilities &FeatureModule::facilities() {
   return *Fac;
 }
 
+void FeatureModuleSet::add(std::unique_ptr<FeatureModule> M) {
+  Modules.push_back(std::move(M));
+}
+
 bool FeatureModuleSet::addImpl(void *Key, std::unique_ptr<FeatureModule> M,
                                const char *Source) {
   if (!Map.try_emplace(Key, M.get()).second) {
@@ -33,5 +37,16 @@ bool FeatureModuleSet::addImpl(void *Key, std::unique_ptr<FeatureModule> M,
   return true;
 }
 
+FeatureModuleSet FeatureModuleSet::fromRegistry() {
+  FeatureModuleSet ModuleSet;
+  for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
+    vlog("Adding feature module '{0}' ({1})", E.getName(), E.getDesc());
+    ModuleSet.add(E.instantiate());
+  }
+  return ModuleSet;
+}
+
 } // namespace clangd
 } // namespace clang
+
+LLVM_INSTANTIATE_REGISTRY(clang::clangd::FeatureModuleRegistry)
diff --git a/clang-tools-extra/clangd/FeatureModule.h b/clang-tools-extra/clangd/FeatureModule.h
index 7b6883507be3f..ee65aa8a59ed2 100644
--- a/clang-tools-extra/clangd/FeatureModule.h
+++ b/clang-tools-extra/clangd/FeatureModule.h
@@ -15,6 +15,7 @@
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Registry.h"
 #include <memory>
 #include <optional>
 #include <type_traits>
@@ -143,9 +144,14 @@ class FeatureModule {
 
 /// A FeatureModuleSet is a collection of feature modules installed in clangd.
 ///
-/// Modules can be looked up by type, or used via the FeatureModule interface.
-/// This allows individual modules to expose a public API.
-/// For this reason, there can be only one feature module of each type.
+/// Modules added with explicit type specification can be looked up by type, or
+/// used via the FeatureModule interface. This allows individual modules to
+/// expose a public API. For this reason, there can be only one feature module
+/// of each type.
+///
+/// Modules added using a base class pointer can be used only via the
+/// FeatureModule interface and can't be looked up by type, thus custom public
+/// API (if provided by the module) can't be used.
 ///
 /// The set owns the modules. It is itself owned by main, not ClangdServer.
 class FeatureModuleSet {
@@ -164,6 +170,8 @@ class FeatureModuleSet {
 public:
   FeatureModuleSet() = default;
 
+  static FeatureModuleSet fromRegistry();
+
   using iterator = llvm::pointee_iterator<decltype(Modules)::iterator>;
   using const_iterator =
       llvm::pointee_iterator<decltype(Modules)::const_iterator>;
@@ -172,6 +180,7 @@ class FeatureModuleSet {
   const_iterator begin() const { return const_iterator(Modules.begin()); }
   const_iterator end() const { return const_iterator(Modules.end()); }
 
+  void add(std::unique_ptr<FeatureModule> M);
   template <typename Mod> bool add(std::unique_ptr<Mod> M) {
     return addImpl(&ID<Mod>::Key, std::move(M), LLVM_PRETTY_FUNCTION);
   }
@@ -185,6 +194,8 @@ class FeatureModuleSet {
 
 template <typename Mod> int FeatureModuleSet::ID<Mod>::Key;
 
+using FeatureModuleRegistry = llvm::Registry<FeatureModule>;
+
 } // namespace clangd
 } // namespace clang
 #endif
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index f287439f10cab..1856d4f47ffc5 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -13,6 +13,7 @@
 #include "Config.h"
 #include "ConfigProvider.h"
 #include "Feature.h"
+#include "FeatureModule.h"
 #include "IncludeCleaner.h"
 #include "PathMapping.h"
 #include "Protocol.h"
@@ -1017,6 +1018,10 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
 
+  FeatureModuleSet ModuleSet = FeatureModuleSet::fromRegistry();
+  if (ModuleSet.begin() != ModuleSet.end())
+    Opts.FeatureModules = &ModuleSet;
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();

@ArcsinX ArcsinX requested a review from kadircet September 3, 2025 05:52
@ArcsinX
Copy link
Contributor Author

ArcsinX commented Sep 17, 2025

@kadircet Sorry to bother you. Can you please take a look on this once again?

Copy link
Member

@kadircet kadircet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot! sorry i got tangled up in couple of other things, hoping to catch up on rest of the reviews soon.

@ArcsinX ArcsinX merged commit 4cabd1e into llvm:main Sep 21, 2025
9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 21, 2025

LLVM Buildbot has detected a new failure on builder clang-m68k-linux-cross running on suse-gary-m68k-cross while building clang-tools-extra at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/16372

Here is the relevant piece of the build log for the reference
Step 5 (ninja check 1) failure: stage 1 checked (failure)
...
[310/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CXXMethodDecl.cpp.o
[311/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/IntegerLiteral.cpp.o
[312/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp.o
[313/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/QualTypeNamesTest.cpp.o
[314/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/LambdaDefaultCapture.cpp.o
[315/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RewriterTest.cpp.o
[316/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/ASTMatchers/ASTMatchersNodeTest.cpp.o
[317/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/DependencyScanning/DependencyScannerTest.cpp.o
[318/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterGenericRedeclTest.cpp.o
[319/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterTest.cpp.o
FAILED: tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterTest.cpp.o 
/usr/bin/c++ -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_STATIC -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GLIBCXX_USE_CXX11_ABI=1 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/tools/clang/unittests -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/clang/unittests -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/clang/include -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/tools/clang/include -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/stage1/include -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/llvm/include -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/clang/unittests/Tooling -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/third-party/unittest/googletest/include -I/var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/third-party/unittest/googlemock/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-dangling-reference -Wno-redundant-move -Wno-pessimizing-move -Wno-array-bounds -Wno-stringop-overread -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -O3 -DNDEBUG -std=c++17  -Wno-variadic-macros -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -Wno-suggest-override -MD -MT tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterTest.cpp.o -MF tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterTest.cpp.o.d -o tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/AST/ASTImporterTest.cpp.o -c /var/lib/buildbot/workers/suse-gary-m68k-cross/clang-m68k-linux-cross/llvm/clang/unittests/AST/ASTImporterTest.cpp
c++: fatal error: Killed signal terminated program cc1plus
compilation terminated.
[320/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/NestedNameSpecifiers.cpp.o
[321/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/InitListExprPostOrder.cpp.o
[322/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/InitListExprPostOrderNoQueue.cpp.o
[323/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp.o
[324/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/ReplacementsYamlTest.cpp.o
[325/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp.o
[326/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/TemplateArgumentLocTraverser.cpp.o
[327/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp.o
[328/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/ParenExpr.cpp.o
[329/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTestDeclVisitor.cpp.o
[330/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/MemberPointerTypeLoc.cpp.o
[331/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RefactoringActionRulesTest.cpp.o
[332/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp.o
[333/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/LexicallyOrderedRecursiveASTVisitorTest.cpp.o
[334/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/Syntax/TreeTestBase.cpp.o
[335/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/Attr.cpp.o
[336/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RangeSelectorTest.cpp.o
[337/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/ToolingTest.cpp.o
[338/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/ASTMatchers/ASTMatchersNarrowingTest.cpp.o
[339/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RefactoringCallbacksTest.cpp.o
[340/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RefactoringTest.cpp.o
[341/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/SourceCodeBuildersTest.cpp.o
[342/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/SourceCodeTest.cpp.o
[343/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/StencilTest.cpp.o
[344/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTestPostOrderVisitor.cpp.o
[345/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CallbacksUnaryOperator.cpp.o
[346/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CallbacksCompoundAssignOperator.cpp.o
[347/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/Syntax/BuildTreeTest.cpp.o
[348/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CallbacksCallExpr.cpp.o
[349/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CallbacksLeaf.cpp.o
[350/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/RecursiveASTVisitorTests/CallbacksBinaryOperator.cpp.o
[351/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/Tooling/TransformerTest.cpp.o
[352/1193] Building CXX object tools/clang/unittests/CMakeFiles/AllClangUnitTests.dir/ASTMatchers/ASTMatchersTraversalTest.cpp.o
ninja: build stopped: subcommand failed.

ckoparkar added a commit to ckoparkar/llvm-project that referenced this pull request Sep 23, 2025
* main: (1562 commits)
  Document Policy on supporting newer C++ standard in LLVM codebase (llvm#156823)
  [MLIR][Transform][SMT] Introduce transform.smt.constrain_params (llvm#159450)
  Reapply "[compiler-rt] Remove %T from shared object substitutions (llvm#155302)"
  [NFC] [IndVarSimplify] Add non-overflowing usub test (llvm#159683)
  [Github] Remove separate tools checkout from pr-code workflows (llvm#159967)
  [clang] fix using enum redecl in template regression (llvm#159996)
  [DAG] Skip `mstore` combine for `<1 x ty>` vectors (llvm#159915)
  [mlir] Expose optional `PatternBenefit` to `func` populate functions (NFC) (llvm#159986)
  [LV] Set correct costs for interleave group members.
  [clang] ast-dump: use template pattern for `instantiated_from` (llvm#159952)
  [ARM] ha-alignstack-call.ll - regenerate test checks (llvm#159988)
  [LLD][MachO] Silence warning when building with MSVC
  [llvm][Analysis] Silence warning when building with MSVC
  [LV] Skip select cost for invariant divisors in legacy cost model.
  [Clang] Fix an error-recovery crash after d1a80de (llvm#159976)
  [VPlanPatternMatch] Introduce m_ConstantInt (llvm#159558)
  [GlobalISel] Add G_ABS computeKnownBits (llvm#154413)
  [gn build] Port 4cabd1e
  Reland "[clangd] Add feature modules registry" (llvm#154836)
  [LV] Also handle non-uniform scalarized loads when processing AddrDefs.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants