Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update llvm::Registry to work for LLVM shared library builds on windows #109024

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

fsfod
Copy link
Contributor

@fsfod fsfod commented Sep 17, 2024

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL.

Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed.
I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by #108276

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:support llvm:ir labels Sep 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 17, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-llvm-ir

Author: Thomas Fransham (fsfod)

Changes

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL.

Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed.
I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by #108276


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

6 Files Affected:

  • (modified) clang/include/clang/Basic/ParsedAttrInfo.h (+5)
  • (modified) clang/include/clang/Frontend/FrontendPluginRegistry.h (+5)
  • (modified) llvm/include/llvm/CodeGen/GCMetadataPrinter.h (+2)
  • (modified) llvm/include/llvm/IR/GCStrategy.h (+2)
  • (modified) llvm/include/llvm/Support/Compiler.h (+5)
  • (modified) llvm/include/llvm/Support/Registry.h (+30-21)
diff --git a/clang/include/clang/Basic/ParsedAttrInfo.h b/clang/include/clang/Basic/ParsedAttrInfo.h
index 537d8f3391d589..2b82dd3b43d133 100644
--- a/clang/include/clang/Basic/ParsedAttrInfo.h
+++ b/clang/include/clang/Basic/ParsedAttrInfo.h
@@ -17,6 +17,7 @@
 
 #include "clang/Basic/AttrSubjectMatchRules.h"
 #include "clang/Basic/AttributeCommonInfo.h"
+#include "clang/Support/Compiler.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Registry.h"
 #include <climits>
@@ -165,4 +166,8 @@ const std::list<std::unique_ptr<ParsedAttrInfo>> &getAttributePluginInstances();
 
 } // namespace clang
 
+namespace llvm {
+extern template class CLANG_TEMPLATE_ABI llvm::Registry<clang::ParsedAttrInfo>;
+} // namespace llvm
+
 #endif // LLVM_CLANG_BASIC_PARSEDATTRINFO_H
diff --git a/clang/include/clang/Frontend/FrontendPluginRegistry.h b/clang/include/clang/Frontend/FrontendPluginRegistry.h
index 810578534acb45..5eea9c2fd89a32 100644
--- a/clang/include/clang/Frontend/FrontendPluginRegistry.h
+++ b/clang/include/clang/Frontend/FrontendPluginRegistry.h
@@ -14,6 +14,7 @@
 #define LLVM_CLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H
 
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Support/Compiler.h"
 #include "llvm/Support/Registry.h"
 
 namespace clang {
@@ -23,4 +24,8 @@ using FrontendPluginRegistry = llvm::Registry<PluginASTAction>;
 
 } // namespace clang
 
+namespace llvm {
+extern template class CLANG_TEMPLATE_ABI Registry<clang::PluginASTAction>;
+} // namespace llvm
+
 #endif // LLVM_CLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H
diff --git a/llvm/include/llvm/CodeGen/GCMetadataPrinter.h b/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
index f9527c9f8752e9..9d421be8313f01 100644
--- a/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
+++ b/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
@@ -34,6 +34,8 @@ class StackMaps;
 /// defaults from Registry.
 using GCMetadataPrinterRegistry = Registry<GCMetadataPrinter>;
 
+extern template class LLVM_TEMPLATE_ABI Registry<GCMetadataPrinter>;
+
 /// GCMetadataPrinter - Emits GC metadata as assembly code.  Instances are
 /// created, managed, and owned by the AsmPrinter.
 class GCMetadataPrinter {
diff --git a/llvm/include/llvm/IR/GCStrategy.h b/llvm/include/llvm/IR/GCStrategy.h
index 3186465f001812..cbfbe23aaa0683 100644
--- a/llvm/include/llvm/IR/GCStrategy.h
+++ b/llvm/include/llvm/IR/GCStrategy.h
@@ -141,6 +141,8 @@ class GCStrategy {
 /// GCMetadataPrinterRegistery as well.
 using GCRegistry = Registry<GCStrategy>;
 
+extern template class LLVM_TEMPLATE_ABI Registry<GCStrategy>;
+
 /// Lookup the GCStrategy object associated with the given gc name.
 std::unique_ptr<GCStrategy> getGCStrategy(const StringRef Name);
 
diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index 1d2d751d4dc11a..e893734b81fb7d 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -179,6 +179,7 @@
 #define LLVM_ABI
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT
 #elif defined(_WIN32) && !defined(__MINGW32__)
 #if defined(LLVM_EXPORTS)
 #define LLVM_ABI __declspec(dllexport)
@@ -189,19 +190,23 @@
 #define LLVM_TEMPLATE_ABI __declspec(dllimport)
 #define LLVM_EXPORT_TEMPLATE
 #endif
+#define LLVM_ABI_EXPORT __declspec(dllexport)
 #elif defined(__ELF__) || defined(__MINGW32__) || defined(_AIX)
 #define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_TEMPLATE_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #elif defined(__MACH__) || defined(__WASM__)
 #define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #endif
 #else
 #define LLVM_ABI
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT
 #endif
 #define LLVM_C_ABI LLVM_ABI
 #endif
diff --git a/llvm/include/llvm/Support/Registry.h b/llvm/include/llvm/Support/Registry.h
index 5bb6a254a47f4c..837cd1237fd38f 100644
--- a/llvm/include/llvm/Support/Registry.h
+++ b/llvm/include/llvm/Support/Registry.h
@@ -53,7 +53,13 @@ namespace llvm {
     Registry() = delete;
 
     friend class node;
-    static node *Head, *Tail;
+    // These must be must two separate declarations to workaround a 20 year
+    // old MSVC bug with dllexport and multiple static fields in the same
+    // declaration causing error C2487 "member of dll interface class may not
+    // be declared with dll interface".
+    // https://developercommunity.visualstudio.com/t/c2487-in-dllexport-class-with-static-members/69878
+    static node *Head;
+    static node *Tail;
 
   public:
     /// Node in linked list of entries.
@@ -134,26 +140,29 @@ namespace llvm {
 /// strictly speaking that's not allowed by the C++ standard (we would need to
 /// have explicit specialization declarations in all translation units where the
 /// specialization is used) so we don't.
-#define LLVM_INSTANTIATE_REGISTRY(REGISTRY_CLASS) \
-  namespace llvm { \
-  template<typename T> typename Registry<T>::node *Registry<T>::Head = nullptr;\
-  template<typename T> typename Registry<T>::node *Registry<T>::Tail = nullptr;\
-  template<typename T> \
-  void Registry<T>::add_node(typename Registry<T>::node *N) { \
-    if (Tail) \
-      Tail->Next = N; \
-    else \
-      Head = N; \
-    Tail = N; \
-  } \
-  template<typename T> typename Registry<T>::iterator Registry<T>::begin() { \
-    return iterator(Head); \
-  } \
-  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Head; \
-  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Tail; \
-  template \
-  void Registry<REGISTRY_CLASS::type>::add_node(REGISTRY_CLASS::node*); \
-  template REGISTRY_CLASS::iterator Registry<REGISTRY_CLASS::type>::begin(); \
+#define LLVM_INSTANTIATE_REGISTRY(REGISTRY_CLASS)                              \
+  namespace llvm {                                                             \
+  template <typename T>                                                        \
+  typename Registry<T>::node *Registry<T>::Head = nullptr;                     \
+  template <typename T>                                                        \
+  typename Registry<T>::node *Registry<T>::Tail = nullptr;                     \
+  template <typename T>                                                        \
+  void Registry<T>::add_node(typename Registry<T>::node *N) {                  \
+    if (Tail)                                                                  \
+      Tail->Next = N;                                                          \
+    else                                                                       \
+      Head = N;                                                                \
+    Tail = N;                                                                  \
+  }                                                                            \
+  template <typename T> typename Registry<T>::iterator Registry<T>::begin() {  \
+    return iterator(Head);                                                     \
+  }                                                                            \
+  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Head;         \
+  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Tail;         \
+  template LLVM_ABI_EXPORT void                                                \
+  Registry<REGISTRY_CLASS::type>::add_node(REGISTRY_CLASS::node *);            \
+  template LLVM_ABI_EXPORT REGISTRY_CLASS::iterator                            \
+  Registry<REGISTRY_CLASS::type>::begin();                                     \
   }
 
 #endif // LLVM_SUPPORT_REGISTRY_H

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 17, 2024

@llvm/pr-subscribers-llvm-support

Author: Thomas Fransham (fsfod)

Changes

This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL.

Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed.
I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by #108276


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

6 Files Affected:

  • (modified) clang/include/clang/Basic/ParsedAttrInfo.h (+5)
  • (modified) clang/include/clang/Frontend/FrontendPluginRegistry.h (+5)
  • (modified) llvm/include/llvm/CodeGen/GCMetadataPrinter.h (+2)
  • (modified) llvm/include/llvm/IR/GCStrategy.h (+2)
  • (modified) llvm/include/llvm/Support/Compiler.h (+5)
  • (modified) llvm/include/llvm/Support/Registry.h (+30-21)
diff --git a/clang/include/clang/Basic/ParsedAttrInfo.h b/clang/include/clang/Basic/ParsedAttrInfo.h
index 537d8f3391d589..2b82dd3b43d133 100644
--- a/clang/include/clang/Basic/ParsedAttrInfo.h
+++ b/clang/include/clang/Basic/ParsedAttrInfo.h
@@ -17,6 +17,7 @@
 
 #include "clang/Basic/AttrSubjectMatchRules.h"
 #include "clang/Basic/AttributeCommonInfo.h"
+#include "clang/Support/Compiler.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Registry.h"
 #include <climits>
@@ -165,4 +166,8 @@ const std::list<std::unique_ptr<ParsedAttrInfo>> &getAttributePluginInstances();
 
 } // namespace clang
 
+namespace llvm {
+extern template class CLANG_TEMPLATE_ABI llvm::Registry<clang::ParsedAttrInfo>;
+} // namespace llvm
+
 #endif // LLVM_CLANG_BASIC_PARSEDATTRINFO_H
diff --git a/clang/include/clang/Frontend/FrontendPluginRegistry.h b/clang/include/clang/Frontend/FrontendPluginRegistry.h
index 810578534acb45..5eea9c2fd89a32 100644
--- a/clang/include/clang/Frontend/FrontendPluginRegistry.h
+++ b/clang/include/clang/Frontend/FrontendPluginRegistry.h
@@ -14,6 +14,7 @@
 #define LLVM_CLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H
 
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Support/Compiler.h"
 #include "llvm/Support/Registry.h"
 
 namespace clang {
@@ -23,4 +24,8 @@ using FrontendPluginRegistry = llvm::Registry<PluginASTAction>;
 
 } // namespace clang
 
+namespace llvm {
+extern template class CLANG_TEMPLATE_ABI Registry<clang::PluginASTAction>;
+} // namespace llvm
+
 #endif // LLVM_CLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H
diff --git a/llvm/include/llvm/CodeGen/GCMetadataPrinter.h b/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
index f9527c9f8752e9..9d421be8313f01 100644
--- a/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
+++ b/llvm/include/llvm/CodeGen/GCMetadataPrinter.h
@@ -34,6 +34,8 @@ class StackMaps;
 /// defaults from Registry.
 using GCMetadataPrinterRegistry = Registry<GCMetadataPrinter>;
 
+extern template class LLVM_TEMPLATE_ABI Registry<GCMetadataPrinter>;
+
 /// GCMetadataPrinter - Emits GC metadata as assembly code.  Instances are
 /// created, managed, and owned by the AsmPrinter.
 class GCMetadataPrinter {
diff --git a/llvm/include/llvm/IR/GCStrategy.h b/llvm/include/llvm/IR/GCStrategy.h
index 3186465f001812..cbfbe23aaa0683 100644
--- a/llvm/include/llvm/IR/GCStrategy.h
+++ b/llvm/include/llvm/IR/GCStrategy.h
@@ -141,6 +141,8 @@ class GCStrategy {
 /// GCMetadataPrinterRegistery as well.
 using GCRegistry = Registry<GCStrategy>;
 
+extern template class LLVM_TEMPLATE_ABI Registry<GCStrategy>;
+
 /// Lookup the GCStrategy object associated with the given gc name.
 std::unique_ptr<GCStrategy> getGCStrategy(const StringRef Name);
 
diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index 1d2d751d4dc11a..e893734b81fb7d 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -179,6 +179,7 @@
 #define LLVM_ABI
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT
 #elif defined(_WIN32) && !defined(__MINGW32__)
 #if defined(LLVM_EXPORTS)
 #define LLVM_ABI __declspec(dllexport)
@@ -189,19 +190,23 @@
 #define LLVM_TEMPLATE_ABI __declspec(dllimport)
 #define LLVM_EXPORT_TEMPLATE
 #endif
+#define LLVM_ABI_EXPORT __declspec(dllexport)
 #elif defined(__ELF__) || defined(__MINGW32__) || defined(_AIX)
 #define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_TEMPLATE_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #elif defined(__MACH__) || defined(__WASM__)
 #define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
 #endif
 #else
 #define LLVM_ABI
 #define LLVM_TEMPLATE_ABI
 #define LLVM_EXPORT_TEMPLATE
+#define LLVM_ABI_EXPORT
 #endif
 #define LLVM_C_ABI LLVM_ABI
 #endif
diff --git a/llvm/include/llvm/Support/Registry.h b/llvm/include/llvm/Support/Registry.h
index 5bb6a254a47f4c..837cd1237fd38f 100644
--- a/llvm/include/llvm/Support/Registry.h
+++ b/llvm/include/llvm/Support/Registry.h
@@ -53,7 +53,13 @@ namespace llvm {
     Registry() = delete;
 
     friend class node;
-    static node *Head, *Tail;
+    // These must be must two separate declarations to workaround a 20 year
+    // old MSVC bug with dllexport and multiple static fields in the same
+    // declaration causing error C2487 "member of dll interface class may not
+    // be declared with dll interface".
+    // https://developercommunity.visualstudio.com/t/c2487-in-dllexport-class-with-static-members/69878
+    static node *Head;
+    static node *Tail;
 
   public:
     /// Node in linked list of entries.
@@ -134,26 +140,29 @@ namespace llvm {
 /// strictly speaking that's not allowed by the C++ standard (we would need to
 /// have explicit specialization declarations in all translation units where the
 /// specialization is used) so we don't.
-#define LLVM_INSTANTIATE_REGISTRY(REGISTRY_CLASS) \
-  namespace llvm { \
-  template<typename T> typename Registry<T>::node *Registry<T>::Head = nullptr;\
-  template<typename T> typename Registry<T>::node *Registry<T>::Tail = nullptr;\
-  template<typename T> \
-  void Registry<T>::add_node(typename Registry<T>::node *N) { \
-    if (Tail) \
-      Tail->Next = N; \
-    else \
-      Head = N; \
-    Tail = N; \
-  } \
-  template<typename T> typename Registry<T>::iterator Registry<T>::begin() { \
-    return iterator(Head); \
-  } \
-  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Head; \
-  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Tail; \
-  template \
-  void Registry<REGISTRY_CLASS::type>::add_node(REGISTRY_CLASS::node*); \
-  template REGISTRY_CLASS::iterator Registry<REGISTRY_CLASS::type>::begin(); \
+#define LLVM_INSTANTIATE_REGISTRY(REGISTRY_CLASS)                              \
+  namespace llvm {                                                             \
+  template <typename T>                                                        \
+  typename Registry<T>::node *Registry<T>::Head = nullptr;                     \
+  template <typename T>                                                        \
+  typename Registry<T>::node *Registry<T>::Tail = nullptr;                     \
+  template <typename T>                                                        \
+  void Registry<T>::add_node(typename Registry<T>::node *N) {                  \
+    if (Tail)                                                                  \
+      Tail->Next = N;                                                          \
+    else                                                                       \
+      Head = N;                                                                \
+    Tail = N;                                                                  \
+  }                                                                            \
+  template <typename T> typename Registry<T>::iterator Registry<T>::begin() {  \
+    return iterator(Head);                                                     \
+  }                                                                            \
+  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Head;         \
+  template REGISTRY_CLASS::node *Registry<REGISTRY_CLASS::type>::Tail;         \
+  template LLVM_ABI_EXPORT void                                                \
+  Registry<REGISTRY_CLASS::type>::add_node(REGISTRY_CLASS::node *);            \
+  template LLVM_ABI_EXPORT REGISTRY_CLASS::iterator                            \
+  Registry<REGISTRY_CLASS::type>::begin();                                     \
   }
 
 #endif // LLVM_SUPPORT_REGISTRY_H

clang/include/clang/Basic/ParsedAttrInfo.h Outdated Show resolved Hide resolved
@@ -189,19 +190,23 @@
#define LLVM_TEMPLATE_ABI __declspec(dllimport)
#define LLVM_EXPORT_TEMPLATE
#endif
#define LLVM_ABI_EXPORT __declspec(dllexport)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being added? I'm weary of any macro that is always export only because the value needs to switch between the definition and reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its macro I mentioned earlier initial explanation about being used in Clang and LLVM, it should never be used for things that could be references.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is subtle enough that we may need some documentation comments explaining when and how someone should use this. (Most reviewers aren't going to be well-versed in how shared libraries work on Windows.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to try and explains its use and behaviour.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, I'm super excited for the ability to write plugins that work on Windows!

@@ -189,19 +190,23 @@
#define LLVM_TEMPLATE_ABI __declspec(dllimport)
#define LLVM_EXPORT_TEMPLATE
#endif
#define LLVM_ABI_EXPORT __declspec(dllexport)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is subtle enough that we may need some documentation comments explaining when and how someone should use this. (Most reviewers aren't going to be well-versed in how shared libraries work on Windows.)

@fsfod
Copy link
Contributor Author

fsfod commented Sep 20, 2024

@AaronBallman I've added some missing extern template entries for Clang. clang-tools-extra also needs some idk if you ok adding them to this PR as well.

@fsfod
Copy link
Contributor Author

fsfod commented Oct 2, 2024

@john-brawn-arm This may be hard to remember, but this a path you've tread before could the LLVM_INSTANTIATE_REGISTRY just be changed to a fulll class explicit template instantiation?

@john-brawn-arm
Copy link
Collaborator

@john-brawn-arm This may be hard to remember, but this a path you've tread before could the LLVM_INSTANTIATE_REGISTRY just be changed to a fulll class explicit template instantiation?

It looks like the approach you're going for is:

  • Registry is declared as extern template in header file. In clang.dll it has no explicit import/export, in plugin.dll it is dllimport
  • In a .cpp file in clang there is an explicit instantiation of Registry with dllexport

Yes, I think LLVM_INSTANTIATE_REGISTRY can just be a explicit instantiation of the template class, as using extern template in the header gets rid of the reason that the current implementation exists.

Copy link

github-actions bot commented Oct 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

These changes LGTM but precommit CI was unable to run, so it would be nice if that could be re-run to ensure it comes back green before landing.

@fsfod
Copy link
Contributor Author

fsfod commented Oct 9, 2024

These changes LGTM but precommit CI was unable to run, so it would be nice if that could be re-run to ensure it comes back green before landing.

It won't be able to pass until #108276 is merged.

fsfod and others added 8 commits October 15, 2024 20:56
Windows doesn't implicitly import and merge exported symbols across shared libraries
like Linux does so we need to explicitly export/import each instantiation of llvm::Registry.
Updated LLVM_INSTANTIATE_REGISTRY macro to export Registry::add_node and
Registry::begin() and add extern template declarations for the all instantiations of llvm::Registry.
Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
…n and CompilationDatabasePlugin registry in clang
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, @fsfod!

@vgvassilev vgvassilev merged commit 00cd1a0 into llvm:main Oct 16, 2024
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 16, 2024

LLVM Buildbot has detected a new failure on builder flang-aarch64-libcxx running on linaro-flang-aarch64-libcxx while building clang,llvm at step 5 "build-unified-tree".

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

Here is the relevant piece of the build log for the reference
Step 5 (build-unified-tree) failure: build (failure)
...
334.480 [204/11/7018] Linking CXX shared library lib/libFortranLower.so.20.0git
334.488 [203/11/7019] Creating library symlink lib/libFortranLower.so
337.442 [203/10/7020] Building CXX object tools/clang/lib/ARCMigrate/CMakeFiles/obj.clangARCMigrate.dir/Transforms.cpp.o
341.192 [203/9/7021] Building CXX object tools/clang/tools/clang-scan-deps/CMakeFiles/clang-scan-deps.dir/ClangScanDeps.cpp.o
342.381 [203/8/7022] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaTemplate.cpp.o
342.732 [203/7/7023] Building CXX object tools/clang/tools/libclang/CMakeFiles/libclang.dir/CXExtractAPI.cpp.o
343.647 [203/6/7024] Building CXX object tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/WebKit/RefCntblBaseVirtualDtorChecker.cpp.o
344.092 [203/5/7025] Building CXX object tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/WebKit/UncountedCallArgsChecker.cpp.o
352.004 [203/4/7026] Building CXX object tools/clang/lib/StaticAnalyzer/Frontend/CMakeFiles/obj.clangStaticAnalyzerFrontend.dir/AnalysisConsumer.cpp.o
369.586 [203/3/7027] Building CXX object tools/flang/lib/FrontendTool/CMakeFiles/flangFrontendTool.dir/ExecuteCompilerInvocation.cpp.o
FAILED: tools/flang/lib/FrontendTool/CMakeFiles/flangFrontendTool.dir/ExecuteCompilerInvocation.cpp.o 
/usr/local/bin/c++ -DFLANG_INCLUDE_TESTS=1 -DFLANG_LITTLE_ENDIAN=1 -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/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/flang/lib/FrontendTool -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/lib/FrontendTool -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/flang/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/include -I/home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/../mlir/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/mlir/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/build/tools/clang/include -isystem /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -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 -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-ctad-maybe-unsupported -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3 -DNDEBUG -std=c++17 -fPIC  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/flang/lib/FrontendTool/CMakeFiles/flangFrontendTool.dir/ExecuteCompilerInvocation.cpp.o -MF tools/flang/lib/FrontendTool/CMakeFiles/flangFrontendTool.dir/ExecuteCompilerInvocation.cpp.o.d -o tools/flang/lib/FrontendTool/CMakeFiles/flangFrontendTool.dir/ExecuteCompilerInvocation.cpp.o -c /home/tcwg-buildbot/worker/flang-aarch64-libcxx/llvm-project/flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
In file included from ../llvm-project/flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20:
In file included from ../llvm-project/flang/include/flang/Frontend/FrontendPluginRegistry.h:19:
../llvm-project/llvm/include/llvm/Support/Registry.h:110:47: error: instantiation of variable 'llvm::Registry<Fortran::frontend::PluginParseTreeAction>::Head' required here, but no definition is available [-Werror,-Wundefined-var-template]
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
../llvm-project/llvm/include/llvm/Support/Registry.h:114:25: note: in instantiation of member function 'llvm::Registry<Fortran::frontend::PluginParseTreeAction>::begin' requested here
  114 |       return make_range(begin(), end());
      |                         ^
../llvm-project/flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:90:34: note: in instantiation of member function 'llvm::Registry<Fortran::frontend::PluginParseTreeAction>::entries' requested here
   90 |          FrontendPluginRegistry::entries()) {
      |                                  ^
../llvm-project/llvm/include/llvm/Support/Registry.h:61:18: note: forward declaration of template entity is here
   61 |     static node *Head;
      |                  ^
../llvm-project/llvm/include/llvm/Support/Registry.h:110:47: note: add an explicit instantiation declaration to suppress this warning if 'llvm::Registry<Fortran::frontend::PluginParseTreeAction>::Head' is explicitly instantiated in another translation unit
  110 |     static iterator begin() { return iterator(Head); }
      |                                               ^
1 error generated.
377.638 [203/2/7028] Building CXX object tools/flang/lib/Frontend/CMakeFiles/flangFrontend.dir/FrontendAction.cpp.o
381.022 [203/1/7029] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaExpr.cpp.o
ninja: build stopped: subcommand failed.

vgvassilev added a commit that referenced this pull request Oct 16, 2024
…on windows (#109024)"

This reverts commit 00cd1a0.

This effectively reverts #109024
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 16, 2024
…ws (llvm#109024)

This is part of the effort to support for enabling plugins on windows by
adding better support for building llvm and clang as a DLL.

Since windows doesn't implicitly import and merge exported symbols
across shared libraries like other platforms we need to explicitly add a
extern template declaration for each instantiation of llvm::Registry to
force the registry symbols to be dllimport'ed.
I've added a new visibility macro that doesn't switch between dllimport
and dllexport on windows since the existing macro would be in the wrong
mode for llvm::Registry's declared in Clang. This PR also depends Clang
symbol visibility macros that will be added by llvm#108276

---------

Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 16, 2024
fsfod added a commit to fsfod/llvm-project that referenced this pull request Oct 17, 2024
…on windows' (llvm#109024)

Fix missing extern templates for llvm::Registry use in other projects of llvm

Windows doesn't implicitly import and merge exported symbols across shared libraries
like Linux does so we need to explicitly export/import each instantiation of llvm::Registry.
Updated LLVM_INSTANTIATE_REGISTRY to just be a full explicit template instantiation.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and LLVM plugins on window.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
…ws (llvm#109024)

This is part of the effort to support for enabling plugins on windows by
adding better support for building llvm and clang as a DLL.

Since windows doesn't implicitly import and merge exported symbols
across shared libraries like other platforms we need to explicitly add a
extern template declaration for each instantiation of llvm::Registry to
force the registry symbols to be dllimport'ed.
I've added a new visibility macro that doesn't switch between dllimport
and dllexport on windows since the existing macro would be in the wrong
mode for llvm::Registry's declared in Clang. This PR also depends Clang
symbol visibility macros that will be added by llvm#108276

---------

Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category llvm:ir llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants