[ItaniumCXXABI] Make tls wrappers properly comdat

[mstorsjo]

Summary

Just marking a symbol as weak_odr/linkonce_odr isn't enough for actually tolerating multiple copies of it at linking on windows, it has to be made a proper comdat.

This should hopefully fix https://bugzilla.mozilla.org/show_bug.cgi?id=1566288.

[rnk]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2519–2520

I would say that this is inaccurate. It greatly affects what the optimizer is allowed to do.

It looks like we forgot to put a comdat on these things, is that not the correct fix?

[mstorsjo]

Actually, this doesn't seem to be enough for the case at hand (used with -ffunction-sections -fdata-sections), I'll have to continue looking into it tomorrow.

[mstorsjo]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2519–2520

Oh, ok.

The full case I was trying to fix (but forgot to recheck after changing this bit) is that when used with -ffunction-sections, the tls wrapper function ends up as comdat one_only, which then gives multiple definition errors. So perhaps the issue is in the handling of -ffunction-sections wrt weak_odr?

[rnk]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2519–2520

Ah, that is an interesting wrinkle. I'm surprised that things worked without -ffunction-sections, though. I would've expected the two plain external definitions to produce multiple definition errors.

[rsmith]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2522

I think the thread wrapper should probably be linkonce_odr across all platforms, at least in all TUs that don't contain a definition of the variable. Every such TU is supposed to provide its own copy regardless, so making it non-discardable seems to serve no purpose.

That said, I suspect this is only hiding the real problem (by discarding the symbol before it creates a link error), and you'd still get link errors if you have two TUs that both use the same thread-local variable and happen to not inline / discard their thread wrappers.

[mstorsjo]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2519–2520

Without -ffunction-sections, there's no separate thread wrapper produced at all, so everything else is generated with working linkage. I just haven't happened to build code that forces generation of a tls wrapper without -ffunction-sections yet.

2522

There's actually a case already where these are made linkonce for other TUs at https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2639-L2642:

// If this isn't a TU in which this variable is defined, the thread
// wrapper is discardable.
if (Wrapper->getLinkage() == llvm::Function::WeakODRLinkage)
  Wrapper->setLinkage(llvm::Function::LinkOnceODRLinkage);

In what cases would two TUs create two non-inline/discardable wrappers - and wouldn't that cause similar linking errors on other platforms as well?

[mstorsjo]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2519–2520

Actually, sorry, I misremembered. Yes, I think the same issue would be present even without -ffunction-sections.

[mstorsjo]

Changed to make it a comdat; unless I'm mistaken, neither linkonce_odr nor weak_odr actually make it possible to link multiple copies of the symbol on windows, unless it's made a comdat. With that in place, the previous change (weak_odr to linkonce_odr) actually isn't needed.

[rnk]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2551

IMO we should be doing the same thing on ELF, you can see the code pattern used elsewhere:
http://llvm-cs.pcc.me.uk/?q=setComdat

Before comdats were made explicit in the IR and we stopped implicitly making comdat groups for odr things on ELF, these wrappers would've been in a comdat group.

[mstorsjo]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2551

Ok, so remove the isOSWindows() and maybe remove the comment altogether as it isn't windows specific any longer?

[mstorsjo]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2551

In the current tests, for linux there's explicit checks that these aren't marked as comdat, see e.g. https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGenCXX/cxx11-thread-local.cpp#L208. Is there a specific reason for that, or should it be changed?

[rnk]

Looks like @rsmith did this here:
https://github.com/llvm/llvm-project/commit/fbe2369f1a514423e4c25417ab3532502fde6f2a

I see that it was replacing a CHECK for a specific comdat group with no comdat at all. I *think* that's not correct, we should be checking for the trivial comdat group, the one for the wrapper function, not the comdat group of the TLS variable. Let's get input from Richard, though.

[mstorsjo]

In D71572#1788786, @rnk wrote:

Looks like @rsmith did this here:
https://github.com/llvm/llvm-project/commit/fbe2369f1a514423e4c25417ab3532502fde6f2a

I see that it was replacing a CHECK for a specific comdat group with no comdat at all. I *think* that's not correct, we should be checking for the trivial comdat group, the one for the wrapper function, not the comdat group of the TLS variable. Let's get input from Richard, though.

@rsmith?

[mstorsjo]

Ping @rsmith

[rsmith]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2551

The test is intending to check that the wrapper is not in a comdat keyed off the variable. Putting it in a trivial comdat is fine and correct; please update the tests to check that it's in the right comdat, not just that it's in some comdat. (I seem to regularly forget that weak / linkonce / weak_odr / linkonce_odr don't actually work as documented any more...)

[mstorsjo]

Making the tls wrappers comdat on any platform that supports comdat; updated tests accordingly.

[mstorsjo]

@rsmith - does this look right to you?

[mstorsjo]

@rnk and/or @rsmith - can either of you have a look at the new version of this?

[rnk]

lgtm

This will be a small change in behavior, but nobody on ELF should notice because things with vague linkage there are both ELF-weak and comdat.

clang/lib/CodeGen/ItaniumCXXABI.cpp

2551

I think this is the confirmation you need, the new tests look correct to me.

clang/lib/CodeGen/ItaniumCXXABI.cpp

getThreadLocalWrapperLinkage(const VarDecl *VD, CodeGen::CodeGenModule &CGM) {
   if (llvm::GlobalValue::isLocalLinkage(VarLinkage))
     return VarLinkage;
 
   // If the thread wrapper is replaceable, give it appropriate linkage.
   if (isThreadWrapperReplaceable(VD, CGM))
     if (!llvm::GlobalVariable::isLinkOnceLinkage(VarLinkage) &&
         !llvm::GlobalVariable::isWeakODRLinkage(VarLinkage))
       return VarLinkage;
   return llvm::GlobalValue::WeakODRLinkage;
 }
 
 llvm::Function *
 ItaniumCXXABI::getOrCreateThreadLocalWrapper(const VarDecl *VD,
                                              llvm::Value *Val) {
   // Mangle the name for the thread_local wrapper function.
   SmallString<256> WrapperName;
   {
     llvm::raw_svector_ostream Out(WrapperName);
     getMangleContext().mangleItaniumThreadLocalWrapper(VD, Out);
   }
ItaniumCXXABI::getOrCreateThreadLocalWrapper(const VarDecl *VD,
   const CGFunctionInfo &FI = CGM.getTypes().arrangeBuiltinFunctionDeclaration(
       getContext().getPointerType(RetQT), FunctionArgList());
 
   llvm::FunctionType *FnTy = CGM.getTypes().GetFunctionType(FI);
   llvm::Function *Wrapper =
       llvm::Function::Create(FnTy, getThreadLocalWrapperLinkage(VD, CGM),
                              WrapperName.str(), &CGM.getModule());
 
+  if (CGM.supportsCOMDAT() && Wrapper->isWeakForLinker())
+    Wrapper->setComdat(CGM.getModule().getOrInsertComdat(Wrapper->getName()));
+
   CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, Wrapper);
 
   // Always resolve references to the wrapper at link time.
   if (!Wrapper->hasLocalLinkage())
     if (!isThreadWrapperReplaceable(VD, CGM) ||
         llvm::GlobalVariable::isLinkOnceLinkage(Wrapper->getLinkage()) ||
         llvm::GlobalVariable::isWeakODRLinkage(Wrapper->getLinkage()) ||
         VD->getVisibility() == HiddenVisibility)

clang/test/CodeGenCXX/cxx11-thread-local-reference.cpp

 // CHECK: call dereferenceable({{[0-9]+}}) i32* @_Z1fv()
 // CHECK: store i32* %{{.*}}, i32** @r, align 8
 
 // CHECK-LABEL: define dereferenceable({{[0-9]+}}) i32* @_Z1gv()
 // LINUX: call i32* @_ZTW1r()
 // DARWIN: call cxx_fast_tlscc i32* @_ZTW1r()
 // CHECK: ret i32* %{{.*}}
 
-// LINUX: define weak_odr hidden i32* @_ZTW1r() [[ATTR0:#[0-9]+]] {
+// LINUX: define weak_odr hidden i32* @_ZTW1r() [[ATTR0:#[0-9]+]] comdat {
 // DARWIN: define cxx_fast_tlscc i32* @_ZTW1r() [[ATTR1:#[0-9]+]] {
 // LINUX: call void @_ZTH1r()
 // DARWIN: call cxx_fast_tlscc void @_ZTH1r()
 // CHECK: load i32*, i32** @r, align 8
 // CHECK: ret i32* %{{.*}}
 
 // LINUX-LABEL: define internal void @__tls_init()
 // DARWIN-LABEL: define internal cxx_fast_tlscc void @__tls_init()
 // CHECK: call void @[[R_INIT]]()
 
 // LINUX: attributes [[ATTR0]] = { {{.*}}"target-features"{{.*}} }
 // DARWIN: attributes [[ATTR1]] = { {{.*}}nounwind{{.*}}"target-features"{{.*}}  }

clang/test/CodeGenCXX/cxx11-thread-local.cpp

 // DARWIN: call {{.*}}_tlv_atexit
 // CHECK: store i64 1, i64* @_ZGVN1XIfE1mE
 // CHECK: br label
 
 // LINUX: declare i32 @__cxa_thread_atexit(void (i8*)*, i8*, i8*)
 // DARWIN: declare i32 @_tlv_atexit(void (i8*)*, i8*, i8*)
 
 // DARWIN: declare cxx_fast_tlscc i32* @_ZTWN1VIcE1mE()
-// LINUX: define linkonce_odr hidden i32* @_ZTWN1VIcE1mE()
-// LINUX-NOT: comdat
+// LINUX: define linkonce_odr hidden i32* @_ZTWN1VIcE1mE() {{#[0-9]+}} comdat {
 // LINUX: br i1 icmp ne (void ()* @_ZTHN1VIcE1mE,
 // LINUX: call void @_ZTHN1VIcE1mE()
 // LINUX: ret i32* @_ZN1VIcE1mE
 
 // DARWIN: declare cxx_fast_tlscc i32* @_ZTWN1WIcE1mE()
-// LINUX: define linkonce_odr hidden i32* @_ZTWN1WIcE1mE()
-// LINUX-NOT: comdat
+// LINUX: define linkonce_odr hidden i32* @_ZTWN1WIcE1mE() {{#[0-9]+}} comdat {
 // LINUX: br i1 icmp ne (void ()* @_ZTHN1WIcE1mE,
 // LINUX: call void @_ZTHN1WIcE1mE()
 // LINUX: ret i32* @_ZN1WIcE1mE
 
 // DARWIN: declare cxx_fast_tlscc {{.*}}* @_ZTWN1XIcE1mE()
-// LINUX: define linkonce_odr hidden {{.*}}* @_ZTWN1XIcE1mE()
-// LINUX-NOT: comdat
+// LINUX: define linkonce_odr hidden {{.*}}* @_ZTWN1XIcE1mE() {{#[0-9]+}} comdat {
 // LINUX: br i1 icmp ne (void ()* @_ZTHN1XIcE1mE,
 // LINUX: call void @_ZTHN1XIcE1mE()
 // LINUX: ret {{.*}}* @_ZN1XIcE1mE
 
 struct S { S(); ~S(); };
 struct T { ~T(); };
 
 // CHECK-LABEL: define void @_Z8tls_dtorv()

clang/test/CodeGenCXX/tls-init-funcs.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.8 -std=c++1y -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -std=c++1y -S -emit-llvm %s -o - | FileCheck %s --check-prefix=MINGW
 
 // CHECK: @a = internal thread_local global
 // CHECK: @_Z2vtIiE = linkonce_odr thread_local global i32 5
 // CHECK: @_ZZ3inlvE3loc = linkonce_odr thread_local global i32 0
 // CHECK: @_tlv_atexit({{.*}}@_ZN1AD1Ev
 // CHECK: call cxx_fast_tlscc i32* @_ZTW3ext()
 // CHECK: declare cxx_fast_tlscc i32* @_ZTW3ext()
 // CHECK-DAG: define weak_odr hidden cxx_fast_tlscc i32* @_ZTW2vtIiE()
 // CHECK-DAG: define weak_odr hidden cxx_fast_tlscc i32* @_ZTW2vtIvE()
 // CHECK-DAG: define {{.*}} @_ZTW1a
 
+// MINGW-DAG: define weak_odr hidden i32* @_ZTW2vtIiE() {{.*}} comdat
+// MINGW-DAG: define weak_odr hidden i32* @_ZTW2vtIvE() {{.*}} comdat
+
 struct A {
   ~A();
 };
 
 thread_local A a;
 
 extern thread_local int ext;
 int &get_ext() { return ext; }

clang/test/OpenMP/threadprivate_codegen.cpp

 // CHECK-TLS: define {{.*}} [[S5]]* [[GS3_TLS_INITD]]
 // CHECK-TLS:   call void [[GS3_TLS_INIT:@[^,]+]]
 // CHECK-TLS:   ret [[S5]]* [[GS3]]
 // CHECK-TLS: }
 // CHECK-TLS: define {{.*}} [2 x [3 x [[S1]]]]* [[ARR_X_TLS_INITD]]
 // CHECK-TLS:   call void [[ARR_X_TLS_INIT]]
 // CHECK-TLS:   ret [2 x [3 x [[S1]]]]* [[ARR_X]]
 // CHECK-TLS: }
-// CHECK-TLS: define {{.*}} i32* [[ST_INT_ST_TLS_INITD]] {{#[0-9]+}} {
+// CHECK-TLS: define {{.*}} i32* [[ST_INT_ST_TLS_INITD]] {{#[0-9]+}} comdat {
 // CHECK-TLS-NOT:   call
 // CHECK-TLS:   ret i32* [[ST_INT_ST]]
 // CHECK-TLS: }
-// CHECK-TLS: define {{.*}} float* [[ST_FLOAT_ST_TLS_INITD]] {{#[0-9]+}} {
+// CHECK-TLS: define {{.*}} float* [[ST_FLOAT_ST_TLS_INITD]] {{#[0-9]+}} comdat {
 // CHECK-TLS-NOT:   call
 // CHECK-TLS:   ret float* [[ST_FLOAT_ST]]
 // CHECK-TLS: }
-// CHECK-TLS: define {{.*}} [[S4]]* [[ST_S4_ST_TLS_INITD]] {{#[0-9]+}} {
+// CHECK-TLS: define {{.*}} [[S4]]* [[ST_S4_ST_TLS_INITD]] {{#[0-9]+}} comdat {
 // CHECK-TLS:   call void [[ST_S4_ST_TLS_INIT]]
 // CHECK-TLS:   ret [[S4]]* [[ST_S4_ST]]
 // CHECK-TLS: }
 
 #endif
 
 #ifdef BODY
 // CHECK-LABEL:  @{{.*}}foobar{{.*}}()