MS ABI: handle inline static data member and inline variable as template static data member

[jyu2]

Summary

MS only run time problem with inline static data member or inline variable.
An inline static data member's initialize function or inline variable's initialize function gets called multiple time.

To fix this, using template static data member’s initialization method instead.
So that inline static data member or inline variable initialize's function can be put into COMDAT group with global being initialized. And also put inline data in the linker directive. So that the function can be called before main.

The bug is report in:
https://bugs.llvm.org/show_bug.cgi?id=37903

[rnk]

Thanks, I think there's another case to handle, though.

lib/CodeGen/CGDeclCXX.cpp

471–473

I think the whole second condition can be simplified to just D->isIinlineSpecified(). We have to consider plain inline global variables, not just static data members, as in:

int f();
inline int gvinit_inline = f();
int useit() { return gvinit_inline ; }

[jyu2]

In D60912#1472983, @rnk wrote:

Thanks, I think there's another case to handle, though.

Hi Reid,
Thank you so much for the review.

Yes, I check the following test case, both gnu and cl call foo twice. Do we want to do differently with that? Thanks. Jennifer
I add MS only, since we are generate same with gnu. Sure remove that should work at run time. But I am not sure if we have abi issues when mix-match object. I will look more about that. Thanks.

bug-37903>g++ x.cpp x1.cpp -std=c++17
bug-37903>./a.out
foo
foo

bug-37903>cat x.h
extern "C" int printf(const char*,...);
int foo();
inline static int aoo = foo(); // C++17 inline variable, thus also a definition
bug-37903>cat x.cpp
#include "x.h"
int foo()
{

printf("foo\n");
return 1;

}
int main()
{
}
bug-37903>cat x1.cpp
#include "x.h"

[rnk]

In D60912#1472986, @jyu2 wrote:

inline static int aoo = foo(); // C++17 inline variable, thus also a definition

This is a static inline global variable, so it technically creates two different globals, so calling foo twice is intended behavior. I think we really want to look at the GVA linkage instead of trying to list all the reasons why something might have weak linkage. Take a look at shouldBeInCOMDAT in CodeGenModule.cpp, I think it has the logic we want.

[jyu2]

In D60912#1472987, @rnk wrote:

In D60912#1472986, @jyu2 wrote:

inline static int aoo = foo(); // C++17 inline variable, thus also a definition

This is a static inline global variable, so it technically creates two different globals, so calling foo twice is intended behavior. I think we really want to look at the GVA linkage instead of trying to list all the reasons why something might have weak linkage. Take a look at shouldBeInCOMDAT in CodeGenModule.cpp, I think it has the logic we want.

Yes, that is good. I will do that way. Thanks.
Jennifer

[jyu2]

Hi Reid,
Thanks for your review and catch what I were missing for inline variable part(only inline static data member were fix in previous patch).
As you suggested, instead using GVA_DiscardableODR to put inline variable's initialization function or inline static member's initialization functions into to COMDAT group with global being initialized.

The GVA_DiscardableODR is for weak linkage of inline variables.

Let me know if you see problems.
Thank you so much!
Thanks.

[rnk]

lgtm

lib/CodeGen/CGDeclCXX.cpp

470–474

I think this can be simplified by removing the isTemplateInstantiation check and then checking for GVA_DiscardableODR or GVA_StrongODR. That will handle the cases of explicit template instantiation that you have below.

It's up to you if you want to implement the simplification. The current code is correct, I don't believe it's possible to create a GVA_StrongODR global variable with a dynamic initializer without template instantiation (phew).

[rnk]

+@hans, per https://bugs.llvm.org/show_bug.cgi?id=37903#c16

[jyu2]

lib/CodeGen/CGDeclCXX.cpp

470–474

Thanks Reid! I also think GVA_StrongODR which is set for template instantiation, but I am not too sure about it. Since we already handle template by calling isTemplateInstantiation, and add GVA_DiscardableODR. I'd like to with this. If we see other problem. We can always add it up.

Thank you so much for your review.

[jyu2]

committed rGc19f4f806972: Fix bug 37903:MS ABI: handle inline static data member and inline variable as… (authored by jyu2).

[MaskRay]

lib/CodeGen/CGDeclCXX.cpp

496

Confirmed with lld-link -subsystem:console -opt:ref -debug:symtab a.o b.o c.o -entry:main -out:win.exe that __declspec(selectany) can be linker GCed.

Sent D106925 to fix it.

lib/CodeGen/CGDeclCXX.cpp

if (D->getTLSKind()) {
     // entire TU.
     CXXThreadLocalInits.push_back(Fn);
     CXXThreadLocalInitVars.push_back(D);
   } else if (PerformInit && ISA) {
     EmitPointerToInitFunc(D, Addr, Fn, ISA);
   } else if (auto *IPA = D->getAttr<InitPriorityAttr>()) {
     OrderGlobalInits Key(IPA->getPriority(), PrioritizedCXXGlobalInits.size());
     PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
-  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind())) {
+  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
+             (getTarget().getCXXABI().isMicrosoft() &&
+              D->isInlineSpecified() && D->isStaticDataMember() &&
+              getLangOpts().CPlusPlus17)) {
     // C++ [basic.start.init]p2:
     //   Definitions of explicitly specialized class template static data
     //   members have ordered initialization. Other class template static data
     //   members (i.e., implicitly or explicitly instantiated specializations)
     //   have unordered initialization.
     //
     // As a consequence, we can put them into their own llvm.global_ctors entry.
     //
     // If the global is externally visible, put the initializer into a COMDAT
     // group with the global being initialized.  On most platforms, this is a
     // minor startup time optimization.  In the MS C++ ABI, there are no guard
     // variables, so this COMDAT key is required for correctness.
     AddGlobalCtor(Fn, 65535, COMDATKey);
+    if (getTarget().getCXXABI().isMicrosoft() && COMDATKey) {
+      // In The MS C++, MS add template static data member in the linker
+      // drective.
+      addUsedGlobal(COMDATKey);
+    }
   } else if (D->hasAttr<SelectAnyAttr>()) {
     // SelectAny globals will be comdat-folded. Put the initializer into a
     // COMDAT group associated with the global, so the initializers get folded
     // too.
     AddGlobalCtor(Fn, 65535, COMDATKey);
   } else {
     I = DelayedCXXInitPosition.find(D); // Re-do lookup in case of re-hash.
     if (I == DelayedCXXInitPosition.end()) {
       CXXGlobalInits.push_back(Fn);
     } else if (I->second != ~0U) {
       assert(I->second < CXXGlobalInits.size() &&
              CXXGlobalInits[I->second] == nullptr);
       CXXGlobalInits[I->second] = Fn;

test/CodeGenCXX/microsoft-abi-template-static-init.cpp

+// RUN: %clang_cc1 %s -triple=i686-pc-win32 -fms-extensions -std=c++17 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-win32 -fms-extensions -std=c++17 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=i686-pc-windows-msvc -fms-extensions -std=c++17 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-windows-msvc -std=c++17 -fms-extensions -emit-llvm -o - | FileCheck %s
+
+struct S {
+  S();
+  ~S();
+};
+
+template <typename T> struct __declspec(dllexport) ExportedTemplate {
+  static S s;
+};
+template <typename T> S ExportedTemplate<T>::s;
+void useExportedTemplate(ExportedTemplate<int> x) {
+  (void)x.s;
+}
+int f();
+namespace selectany_init {
+// MS don't put selectany static var in the linker directive, init routine
+// f() is not getting called if x is not referenced.
+int __declspec(selectany) x = f();
+}
+
+namespace explicit_template_instantiation {
+template <typename T> struct A { static  int x; };
+template <typename T> int A<T>::x = f();
+template struct A<int>;
+}
+
+namespace implicit_template_instantiation {
+template <typename T> struct A { static  int x; };
+template <typename T>  int A<T>::x = f();
+int g() { return A<int>::x; }
+}
+
+
+template <class T>
+struct X_ {
+  static T ioo;
+  static T init();
+};
+template <class T> T X_<T>::ioo = X_<T>::init();
+template struct X_<int>;
+
+template <class T>
+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 {
+public:
+  a();
+};
+// For the static var inside unnamed namespace, the object is local to TU.
+// No need to put static var in the linker directive.
+// The static init routine is called before main.
+namespace {
+template <int> class aj {
+public:
+  static a al;
+};
+template <int am> a aj<am>::al;
+class b : aj<3> {
+  void c();
+};
+void b::c() { al; }
+}
+
+// C++17, inline static data member also need to use
+struct A
+{
+  A();
+  ~A();
+};
+
+struct S1
+{
+  inline static A aoo; // C++17 inline variable, thus also a definition
+};
+
+// CHECK: @llvm.used = appending global [5 x i8*] [i8* bitcast (i32* @"?x@?$A@H@explicit_template_instantiation@@2HA" to i8*), i8* bitcast (i32* @"?ioo@?$X_@H@@2HA" to i8*), i8* getelementptr inbounds (%struct.A, %struct.A* @"?aoo@S1@@2UA@@A", i32 0, i32 0), i8* getelementptr inbounds (%struct.S, %struct.S* @"?s@?$ExportedTemplate@H@@2US@@A", i32 0, i32 0), i8* bitcast (i32* @"?x@?$A@H@implicit_template_instantiation@@2HA" to i8*)], section "llvm.metadata"