Emit initializers for selectany globals in comdat-associative sections (PR20889)

[hans]

Summary

This prevents initializers for comdat-folded globals from running multiple times.

Please take a look!

[rnk]

lib/CodeGen/CGDeclCXX.cpp

335 ↗

(On Diff #13505)

Should we do the same for WeakAttr? What does GCC do?

I wonder if there is a more general GVA linkage check we should do. Can you add a test for static data members of implicitly instantiated dllexport class templates?

[hans]

lib/CodeGen/CGDeclCXX.cpp

335 ↗

(On Diff #13505)

Yes, doing the same for WeakAttr sounds reasonable.

I couldn't observe gcc doing anything special for dynamic initializers of weak objects :/

We do get this right for static data members of implicitly instantiated class templates; it's handled by your code above, and dllexport doesn't change it. I can add a test.

If we want to base this on GVA linkage, it seems to me that the associative comdat method should be applied to all symbols that can be merged, so GVA_StrongODR and GVA_DiscardableODR? But I don't think SelectAny or WeakAttr are reflected in the gva linkage.

[rnk]

lgtm

lib/CodeGen/CGDeclCXX.cpp

335 ↗

(On Diff #13505)

I *think* selectany and templates are the only ways to get COMDAT data with dynamic initialization, so we should be OK with what we have then.

[hans]

Closed by commit rL217534 (authored by @hans).

cfe/trunk/lib/CodeGen/CGDeclCXX.cpp

if (D->getTLSKind()) {
     // As a consequence, we can put them into their own llvm.global_ctors entry.
     //
     // In addition, 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, Addr);
     DelayedCXXInitPosition.erase(D);
+  } 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, Addr);
+    DelayedCXXInitPosition.erase(D);
   } else {
     llvm::DenseMap<const Decl *, unsigned>::iterator I =
       DelayedCXXInitPosition.find(D);
     if (I == DelayedCXXInitPosition.end()) {
       CXXGlobalInits.push_back(Fn);
     } else {
       assert(CXXGlobalInits[I->second] == nullptr);
       CXXGlobalInits[I->second] = Fn;

cfe/trunk/test/CodeGenCXX/microsoft-abi-static-initializers.cpp

 // RUN: %clang_cc1 -fms-extensions -emit-llvm %s -o - -mconstructor-aliases -triple=i386-pc-win32 | FileCheck %s
 
-// CHECK: @llvm.global_ctors = appending global [2 x { i32, void ()*, i8* }]
-// CHECK: [{ i32, void ()*, i8* } { i32 65535, void ()* @"\01??__Efoo@?$B@H@@2VA@@A@YAXXZ",
-// CHECK:       i8* bitcast (%class.A* @"\01?foo@?$B@H@@2VA@@A" to i8*) },
-// CHECK:  { i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_microsoft_abi_static_initializers.cpp, i8* null }]
+// CHECK: @llvm.global_ctors = appending global [5 x { i32, void ()*, i8* }] [
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* @"\01??__Eselectany1@@YAXXZ", i8* getelementptr inbounds (%struct.S* @"\01?selectany1@@3US@@A", i32 0, i32 0) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* @"\01??__Eselectany2@@YAXXZ", i8* getelementptr inbounds (%struct.S* @"\01?selectany2@@3US@@A", i32 0, i32 0) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* @"\01??__Es@?$ExportedTemplate@H@@2US@@A@YAXXZ", i8* getelementptr inbounds (%struct.S* @"\01?s@?$ExportedTemplate@H@@2US@@A", i32 0, i32 0) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* @"\01??__Efoo@?$B@H@@2VA@@A@YAXXZ", i8* bitcast (%class.A* @"\01?foo@?$B@H@@2VA@@A" to i8*) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_microsoft_abi_static_initializers.cpp, i8* null }
+// CHECK: ]
 
 struct S {
   S();
   ~S();
 };
 
 S s;
 
 // CHECK: define internal void @"\01??__Es@@YAXXZ"()
 // CHECK: call x86_thiscallcc %struct.S* @"\01??0S@@QAE@XZ"
 // CHECK: call i32 @atexit(void ()* @"\01??__Fs@@YAXXZ")
 // CHECK: ret void
 
 // CHECK: define internal void @"\01??__Fs@@YAXXZ"()
 // CHECK: call x86_thiscallcc void @"\01??1S@@QAE@XZ"
 // CHECK: ret void
 
-// These globals should use distinct guard variables, and not different bits of
-// the same global.
+// These globals should have initializers comdat associative with the global.
+// See @llvm.global_ctors above.
 __declspec(selectany) S selectany1;
 __declspec(selectany) S selectany2;
 // CHECK: define linkonce_odr void @"\01??__Eselectany1@@YAXXZ"()
 // CHECK-NOT: @"\01??_Bselectany1
 // CHECK: call x86_thiscallcc %struct.S* @"\01??0S@@QAE@XZ"
 // CHECK: ret void
 // CHECK: define linkonce_odr void @"\01??__Eselectany2@@YAXXZ"()
 // CHECK-NOT: @"\01??_Bselectany2
 // CHECK: call x86_thiscallcc %struct.S* @"\01??0S@@QAE@XZ"
 // CHECK: ret void
 
+// The implicitly instantiated static data member should have initializer
+// comdat associative with the global.
+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;
+}
+
 void StaticLocal() {
   static S TheS;
 }
+
 // CHECK-LABEL: define void @"\01?StaticLocal@@YAXXZ"()
 // CHECK: load i32* @"\01?$S1@?0??StaticLocal@@YAXXZ@4IA"
 // CHECK: store i32 {{.*}}, i32* @"\01?$S1@?0??StaticLocal@@YAXXZ@4IA"
 // CHECK: ret
 
 void MultipleStatics() {
   static S S1;
   static S S2;