TLS: Respect visibility for thread_local variables on Darwin (PR40327)

[vlad.tsyrklevich]

Summary

Teach clang to mark thread wrappers for thread_local variables with
hidden visibility when the original variable is marked with hidden
visibility. This is necessary on Darwin which exposes the thread wrapper
instead of the thread variable. The thread wrapper would previously
always be created with default visibility unless it had
linkonce*/weak_odr linkage.

[vlad.tsyrklevich]

John I took a brief at look at the visibility for templated thread_local variables on Darwin and they looked correct at a glance, though I'm not confident I understand the ABI well enough to understand exactly how the linkonce*/weak_odr case differs between Darwin and other platforms.

(Also includes a fix for a typo I noticed in https://reviews.llvm.org/rC252814)

[rjmccall]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2471 ↗

(On Diff #182171)

Should we also propagate protected visibility to the wrapper (on non-Darwin targets that support it in the first place)?

[vlad.tsyrklevich]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2471 ↗

(On Diff #182171)

isThreadWrapperReplaceable() is only true on Darwin which is why I ignored that possibility. Are you aware of any other platforms that implement an equivalent thread_local ABI?

[vlad.tsyrklevich]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2471 ↗

(On Diff #182171)

Or perhaps you meant propagate protected visibility to the wrappers in the non-Darwin isThreadWrapperReplaceable()==false case where they're currently always given hidden visibility? I'm not sure why we would ever pass protected visibility down in that case since hidden should always be preferred, unless we're considering the case of local linkage?

[rjmccall]

LGTM.

clang/lib/CodeGen/ItaniumCXXABI.cpp

2471 ↗

(On Diff #182171)

I don't know the thread-local ABIs well enough, it seems. If it's always hidden visibility at best on ELF, it doesn't matter.

lib/CodeGen/ItaniumCXXABI.cpp

llvm::Function *Wrapper =
                              WrapperName.str(), &CGM.getModule());
 
   CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, Wrapper);
 
   if (VD->hasDefinition())
     CGM.SetLLVMFunctionAttributesForDefinition(nullptr, Wrapper);
 
   // Always resolve references to the wrapper at link time.
-  if (!Wrapper->hasLocalLinkage() && !(isThreadWrapperReplaceable(VD, CGM) &&
-      !llvm::GlobalVariable::isLinkOnceLinkage(Wrapper->getLinkage()) &&
-      !llvm::GlobalVariable::isWeakODRLinkage(Wrapper->getLinkage())))
+  if (!Wrapper->hasLocalLinkage())
+    if (!isThreadWrapperReplaceable(VD, CGM) ||
+        llvm::GlobalVariable::isLinkOnceLinkage(Wrapper->getLinkage()) ||
+        llvm::GlobalVariable::isWeakODRLinkage(Wrapper->getLinkage()) ||
+        VD->getVisibility() == HiddenVisibility)
       Wrapper->setVisibility(llvm::GlobalValue::HiddenVisibility);
 
   if (isThreadWrapperReplaceable(VD, CGM)) {
     Wrapper->setCallingConv(llvm::CallingConv::CXX_FAST_TLS);
     Wrapper->addFnAttr(llvm::Attribute::NoUnwind);
   }
   return Wrapper;
 }
 

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

+// RUN: %clang_cc1 -std=c++11 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck --check-prefix=LINUX %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm %s -o - -triple x86_64-apple-darwin12 | FileCheck --check-prefix=DARWIN %s
+
+// Regression test for PR40327
+
+// LINUX: @default_tls = thread_local global i32
+// LINUX: @hidden_tls = hidden thread_local global i32
+// LINUX: define weak_odr hidden i32* @_ZTW11default_tls()
+// LINUX: define weak_odr hidden i32* @_ZTW10hidden_tls()
+//
+// DARWIN: @default_tls = internal thread_local global i32
+// DARWIN: @hidden_tls = internal thread_local global i32
+// DARWIN: define cxx_fast_tlscc i32* @_ZTW11default_tls()
+// DARWIN: define hidden cxx_fast_tlscc i32* @_ZTW10hidden_tls()
+
+__attribute__((visibility("default"))) thread_local int default_tls;
+__attribute__((visibility("hidden"))) thread_local int hidden_tls;

test/CodeGenCXX/cxx11-thread-local.cpp

 // CHECK: call void @[[A_INIT]]()
 // CHECK-NOT: call void @[[V_M_INIT]]()
 // CHECK: call void @[[D_INIT]]()
 // CHECK-NOT: call void @[[V_M_INIT]]()
 // CHECK: call void @[[U_M_INIT]]()
 // CHECK-NOT: call void @[[V_M_INIT]]()
 
 
-// LIUNX: define weak_odr hidden i32* @_ZTW1a() {
+// LINUX: define weak_odr hidden i32* @_ZTW1a()
 // DARWIN: define cxx_fast_tlscc i32* @_ZTW1a()
 // LINUX:   call void @_ZTH1a()
 // DARWIN: call cxx_fast_tlscc void @_ZTH1a()
 // CHECK:   ret i32* @a
 // CHECK: }
 
 
 // LINUX: declare extern_weak void @_ZTH1b() [[ATTR:#[0-9]+]]