[MS] Put dllexported inline global initializers in a comdat #107154
Merged
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
Follow-up to c19f4f8 to handle corner case of exported inline variables. Should fix llvm#56485
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Reid Kleckner (rnk) ChangesFollow-up to c19f4f8 to handle corner case of exported inline variables. Should fix #56485 Full diff: https://github.com/llvm/llvm-project/pull/107154.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGDeclCXX.cpp b/clang/lib/CodeGen/CGDeclCXX.cpp
index 2f56355cff90ec..c9e0fb0d8afaba 100644
--- a/clang/lib/CodeGen/CGDeclCXX.cpp
+++ b/clang/lib/CodeGen/CGDeclCXX.cpp
@@ -586,7 +586,7 @@ CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
PrioritizedCXXGlobalInits.size());
PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
} else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
- getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
+ !isUniqueGVALinkage(getContext().GetGVALinkageForVariable(D)) ||
D->hasAttr<SelectAnyAttr>()) {
// C++ [basic.start.init]p2:
// Definitions of explicitly specialized class template static data
diff --git a/clang/test/CodeGenCXX/microsoft-abi-template-static-init.cpp b/clang/test/CodeGenCXX/microsoft-abi-template-static-init.cpp
index 60b48abca2f89a..871551240debfd 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-template-static-init.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-template-static-init.cpp
@@ -49,8 +49,6 @@ struct X {
static T ioo;
static T init();
};
-// template specialized static data don't need in llvm.used,
-// the static init routine get call from _GLOBAL__sub_I_ routines.
template <> int X<int>::ioo = X<int>::init();
template struct X<int>;
class a {
@@ -87,5 +85,6 @@ struct S1
int foo();
inline int zoo = foo();
inline static int boo = foo();
+inline __declspec(dllexport) A exported_inline{};
-// CHECK: @llvm.used = appending global [8 x ptr] [ptr @"?x@selectany_init@@3HA", ptr @"?x1@selectany_init@@3HA", ptr @"?x@?$A@H@explicit_template_instantiation@@2HA", ptr @"?ioo@?$X_@H@@2HA", ptr @"?aoo@S1@@2UA@@A", ptr @"?zoo@@3HA", ptr @"?s@?$ExportedTemplate@H@@2US@@A", ptr @"?x@?$A@H@implicit_template_instantiation@@2HA"], section "llvm.metadata"
+// CHECK: @llvm.used = appending global [10 x ptr] [ptr @"?x@selectany_init@@3HA", ptr @"?x1@selectany_init@@3HA", ptr @"?x@?$A@H@explicit_template_instantiation@@2HA", ptr @"?ioo@?$X_@H@@2HA", ptr @"?ioo@?$X@H@@2HA", ptr @"?aoo@S1@@2UA@@A", ptr @"?zoo@@3HA", ptr @"?exported_inline@@3UA@@A", ptr @"?s@?$ExportedTemplate@H@@2US@@A", ptr @"?x@?$A@H@implicit_template_instantiation@@2HA"], section "llvm.metadata"
|
|
What's the interaction here with the standard's ordering guarantees? The comment in the code indicates that we can't make a separate comdat for ordered initialization... but inline variables do require partial ordering. Please update the comment as appropriate. |
|
I think there is no change here in our conformance on inline variable initialization order, except that non-discardable inline variables (achieved in this instance with dllexport, but perhaps there are other ways to do this. The classic case is explicit instantiation, which is unordered) are now treated the same as regular inline variables. My reading of the code is that we are currently technically non-conforming. (confirmed, we put inline globals in comdats: https://godbolt.org/z/aW7szsdfE) There is some back and forth on whether this is good or bad elsewhere in the issue tracker, but I'd have to dig it up. |
|
If there isn't a way to emit conforming code, then I think it's fine to emit non-conforming code, as long as there's an appropriate comment. Breaking the ABI is clearly worse. Does this impact non-MS targets? |
|
Yeah, I should've explicitly said I was working on updating the comment block. It needed significant surgery. Actually, this code could probably use more significant refactoring, but let's set that aside for now.
At the moment, I can't think of a good way to make an inline global |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks for the review! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/4907 Here is the relevant piece of the build log for the reference |
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.
Follow-up to c19f4f8 to handle corner case of exported inline variables.
Should fix #56485