Update llvm::Registry to work for LLVM shared library builds on windows #109024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-llvm-ir Author: Thomas Fransham (fsfod) ChangesThis 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. Full diff: https://github.com/llvm/llvm-project/pull/109024.diff 6 Files Affected:
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
|
|
@llvm/pr-subscribers-llvm-support Author: Thomas Fransham (fsfod) ChangesThis 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. Full diff: https://github.com/llvm/llvm-project/pull/109024.diff 6 Files Affected:
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
|
compnerd
reviewed
Sep 18, 2024
| @@ -189,19 +190,23 @@ | |||
| #define LLVM_TEMPLATE_ABI __declspec(dllimport) | |||
| #define LLVM_EXPORT_TEMPLATE | |||
| #endif | |||
| #define LLVM_ABI_EXPORT __declspec(dllexport) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I added a comment to try and explains its use and behaviour.
| @@ -189,19 +190,23 @@ | |||
| #define LLVM_TEMPLATE_ABI __declspec(dllimport) | |||
| #define LLVM_EXPORT_TEMPLATE | |||
| #endif | |||
| #define LLVM_ABI_EXPORT __declspec(dllexport) | |||
There was a problem hiding this comment.
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.)
Open
40 tasks
|
@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. |
|
@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:
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. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
AaronBallman
reviewed
Oct 8, 2024
It won't be able to pass until #108276 is merged. |
vgvassilev
force-pushed
the
exported-api/llvm-registry
branch
from
75c3816 to
b1147fa
Compare
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.
…egistry Fixes Tail': 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
Co-authored-by: Saleem Abdulrasool <[email protected]>
…n and CompilationDatabasePlugin registry in clang
…explicit instantiation
vgvassilev
force-pushed
the
exported-api/llvm-registry
branch
from
c25068a to
5d9d904
Compare
vgvassilev
approved these changes
Oct 16, 2024
|
LLVM Buildbot has detected a new failure on builder 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 |
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.
vgvassilev
pushed a commit
that referenced
this pull request
Oct 22, 2024
…on windows' (#109024) (#112640) 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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