Skip to content

Conversation

@ArcsinX
Copy link
Contributor

@ArcsinX ArcsinX commented Aug 15, 2025

This patch adds feature modules registry, as discussed with @kadircet in discourse.
Feature modules, which added into the feature module set from registry entries, can't expose public API, but still can be used via FeatureModule interface.

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 15, 2025

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

@llvm/pr-subscribers-clangd

Author: Aleksandr Platonov (ArcsinX)

Changes

This patch adds feature modules registry, as discussed with @kadircet in discourse.
Feature modules. which added into the feature module set from registry entries, can't expose public API, but still can be used via FeatureModule interface.


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

3 Files Affected:

  • (modified) clang-tools-extra/clangd/FeatureModule.cpp (+2)
  • (modified) clang-tools-extra/clangd/FeatureModule.h (+14-3)
  • (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+7)
diff --git a/clang-tools-extra/clangd/FeatureModule.cpp b/clang-tools-extra/clangd/FeatureModule.cpp
index 872cea1443789..47aff0b513f0d 100644
--- a/clang-tools-extra/clangd/FeatureModule.cpp
+++ b/clang-tools-extra/clangd/FeatureModule.cpp
@@ -9,6 +9,8 @@
 #include "FeatureModule.h"
 #include "support/Logger.h"
 
+LLVM_INSTANTIATE_REGISTRY(clang::clangd::FeatureModuleRegistry)
+
 namespace clang {
 namespace clangd {
 
diff --git a/clang-tools-extra/clangd/FeatureModule.h b/clang-tools-extra/clangd/FeatureModule.h
index 7b6883507be3f..3697a68ed4454 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 {
@@ -172,6 +178,9 @@ 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) {
+    Modules.push_back(std::move(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;
 
+typedef llvm::Registry<FeatureModule> FeatureModuleRegistry;
+
 } // 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..87c9f701d95ea 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -1017,6 +1017,13 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
 
+  FeatureModuleSet ModuleSet;
+  for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
+    vlog("Adding feature module '{0}' ({1})", E.getName(), E.getDesc());
+    ModuleSet.add(E.instantiate());
+  }
+  Opts.FeatureModules = &ModuleSet;
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();

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!

@ArcsinX ArcsinX merged commit ff5767a into llvm:main Aug 21, 2025
9 checks passed
ArcsinX added a commit that referenced this pull request Aug 21, 2025
kadircet pushed a commit that referenced this pull request Aug 21, 2025
Reverts #153756

It leads to new build bot failure.
https://lab.llvm.org/buildbot/#/builders/145/builds/9200

```
BUILD FAILED: failed build (failure)

Step 5 (build-unified-tree) failure: build (failure) ...
254.983 [140/55/1504] Building CXX object tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o
FAILED: tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o
ccache /home/buildbots/llvm-external-buildbots/clang.19.1.7/bin/clang++ --gcc-toolchain=/gcc-toolchain/usr -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/../include-cleaner/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd/../clang-tidy -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-
 rhel-test/clang-ppc64le-rhel/build/tools/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe  -unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o -MF tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o.d -o tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o -c /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:10:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ClangdServer.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ASTSignals.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ParsedAST.h:23:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/CollectMacros.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/Protocol.h:26:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/URI.h:14:
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:110:47: error: instantiation of variable 'llvm::Registry<clang::clangd::FeatureModule>::Head' required here, but no definition is available [-Werror,-Wundefined-var-template]
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:114:25: note: in instantiation of member function 'llvm::Registry<clang::clangd::FeatureModule>::begin' requested here
  114 |       return make_range(begin(), end());
      |                         ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:1021:64: note: in instantiation of member function 'llvm::Registry<clang::clangd::FeatureModule>::entries' requested here
 1021 |   for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
      |                                                                ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:61:18: note: forward declaration of template entity is here
   61 |     static node *Head;
      |                  ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:110:47: note: add an explicit instantiation declaration to suppress this warning if 'llvm::Registry<clang::clangd::FeatureModule>::Head' is explicitly instantiated in another translation unit
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
1 error generated.
```

I need some time to fix this in a correct way
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Aug 21, 2025
Reverts llvm/llvm-project#153756

It leads to new build bot failure.
https://lab.llvm.org/buildbot/#/builders/145/builds/9200

```
BUILD FAILED: failed build (failure)

Step 5 (build-unified-tree) failure: build (failure) ...
254.983 [140/55/1504] Building CXX object tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o
FAILED: tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o
ccache /home/buildbots/llvm-external-buildbots/clang.19.1.7/bin/clang++ --gcc-toolchain=/gcc-toolchain/usr -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/../include-cleaner/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd/../clang-tidy -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-
 rhel-test/clang-ppc64le-rhel/build/tools/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/clangd -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe  -unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o -MF tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o.d -o tools/clang/tools/extra/clangd/tool/CMakeFiles/obj.clangdMain.dir/ClangdMain.cpp.o -c /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:10:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ClangdServer.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ASTSignals.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/ParsedAST.h:23:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/CollectMacros.h:12:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/Protocol.h:26:
In file included from /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/URI.h:14:
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:110:47: error: instantiation of variable 'llvm::Registry<clang::clangd::FeatureModule>::Head' required here, but no definition is available [-Werror,-Wundefined-var-template]
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:114:25: note: in instantiation of member function 'llvm::Registry<clang::clangd::FeatureModule>::begin' requested here
  114 |       return make_range(begin(), end());
      |                         ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:1021:64: note: in instantiation of member function 'llvm::Registry<clang::clangd::FeatureModule>::entries' requested here
 1021 |   for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
      |                                                                ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:61:18: note: forward declaration of template entity is here
   61 |     static node *Head;
      |                  ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include/llvm/Support/Registry.h:110:47: note: add an explicit instantiation declaration to suppress this warning if 'llvm::Registry<clang::clangd::FeatureModule>::Head' is explicitly instantiated in another translation unit
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
1 error generated.
```

I need some time to fix this in a correct way
ArcsinX added a commit that referenced this pull request Sep 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.
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.

3 participants