[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: lib/CodeGen/CGDeclCXX.cpp:494
 // too.
 AddGlobalCtor(Fn, 65535, COMDATKey);
   } else {

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 closed this revision.
jyu2 added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 marked an inline comment as done.
jyu2 added inline comments.



Comment at: lib/CodeGen/CGDeclCXX.cpp:470-471
 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
-  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind())) {
+  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
+ getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR) {
 // C++ [basic.start.init]p2:

rnk wrote:
> 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).
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.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: hans.
rnk added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: lib/CodeGen/CGDeclCXX.cpp:470-471
 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
-  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind())) {
+  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
+ getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR) {
 // C++ [basic.start.init]p2:

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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60912: MS ABI: handle inline static data member and inline variable as template static data member

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 updated this revision to Diff 196346.
jyu2 retitled this revision from "MS ABI: handle inline static data member as 
template static data member" to "MS ABI: handle inline static data member and 
inline variable as template static data member".
jyu2 edited the summary of this revision.
jyu2 added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60912/new/

https://reviews.llvm.org/D60912

Files:
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenCXX/microsoft-abi-template-static-init.cpp
  test/Modules/initializers.cpp

Index: test/Modules/initializers.cpp
===
--- test/Modules/initializers.cpp
+++ test/Modules/initializers.cpp
@@ -235,7 +235,6 @@
 
 // CHECK-IMPORT: define {{.*}} @[[TU_INIT]]()
 // CHECK-IMPORT: call void @[[A_INIT]]()
-// CHECK-IMPORT: call void @[[B_INIT]]()
 
 // CHECK-IMPORT: define {{.*}} @__tls_init()
 // CHECK-IMPORT: call void @[[C_INIT]]()
Index: test/CodeGenCXX/microsoft-abi-template-static-init.cpp
===
--- /dev/null
+++ test/CodeGenCXX/microsoft-abi-template-static-init.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 %s -triple=i686-pc-win32 -fms-extensions -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-win32 -fms-extensions -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=i686-pc-windows-msvc -fms-extensions -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-windows-msvc  -fms-extensions -emit-llvm -o - | FileCheck %s
+
+struct S {
+  S();
+  ~S();
+};
+
+template  struct __declspec(dllexport) ExportedTemplate {
+  static S s;
+};
+template  S ExportedTemplate::s;
+void useExportedTemplate(ExportedTemplate 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();
+inline int __declspec(selectany) x1 = f();
+}
+
+namespace explicit_template_instantiation {
+template  struct A { static  int x; };
+template  int A::x = f();
+template struct A;
+}
+
+namespace implicit_template_instantiation {
+template  struct A { static  int x; };
+template   int A::x = f();
+int g() { return A::x; }
+}
+
+
+template 
+struct X_ {
+  static T ioo;
+  static T init();
+};
+template  T X_::ioo = X_::init();
+template struct X_;
+
+template 
+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::ioo = X::init();
+template struct X;
+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  class aj {
+public:
+  static a al;
+};
+template  a aj::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
+};
+
+int foo();
+inline int zoo = foo();
+inline static int boo = foo();
+
+
+// CHECK: @llvm.used = appending global [7 x i8*] [i8* bitcast (i32* @"?x1@selectany_init@@3HA" to 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* bitcast (i32* @"?zoo@@3HA" to i8*), 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"
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -467,7 +467,8 @@
   } else if (auto *IPA = D->getAttr()) {
 OrderGlobalInits Key(IPA->getPriority(), PrioritizedCXXGlobalInits.size());
 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
-  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind())) {
+  } else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
+ getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR) {
 // C++ [basic.start.init]p2:
 //