CodeGen: Don't emit a thread-wrapper if we can't touch the backing variable

[majnemer]

Summary

OS X TLS has all accesses going through the thread-wrapper function and
gives the backing thread-local variable internal linkage. This means
that thread-wrappers must have WeakAnyLinkage so that references to the
internal thread-local variables do not get propagated to other code.

It also means that translation units which do not provide a definition
for the thread-local variable cannot attempt to emit a thread-wrapper
because the thread wrapper will attempt to reference the backing
variable.

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1599

Hmm, what's the motivation for making this weak? Should it be strong if the thread_local variable is a strong definition?

(I'm also not completely sure whether WeakAny or WeakODR is appropriate for the case where the variable *is* weak; I *think* the intent is for the underlying mechanism to be replaceable, which implies using WeakAny.)

test/CodeGenCXX/tls-init-funcs.cpp

6–7

Why are these marked as hidden? Don't they need to be visible to support cross-DSO linking of thread_local variables?

[majnemer]

lib/CodeGen/ItaniumCXXABI.cpp

1599

It's a strong definition but an internal one.

WeakODR permits inlining of the thread-wrapper. This, in turn, allows references to the internal backing store of the variable to be proliferated if the thread-wrapper gets inlined.

test/CodeGenCXX/tls-init-funcs.cpp

6–7

ToT clang makes the thread-wrapper hidden. ItaniumCXXABI.cpp has the following comment in it:

Always resolve references to the wrapper at link time.

It was added by you in r179858.

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1599

I feel like I'm missing something here. Why doesn't this:

thread_local int n = f();

... get a non-weak, inlineable definition for its thread wrapper (in the translation unit with the definition)?

test/CodeGenCXX/tls-init-funcs.cpp

6–7

That works on the basis that every translation unit which references a TW symbol provides a (weak) definition of that symbol. That's not true in the Mac OS case (where they don't follow the Itanium ABI).

[majnemer]

lib/CodeGen/ItaniumCXXABI.cpp

1599

True but we still need to handle cases like:

template <typename T>
thread_local int n = f();

I'll try to make it more conservative.

test/CodeGenCXX/tls-init-funcs.cpp

6–7

Then we can skip the hidden.

[majnemer]

• Address Richard's review comments.

[majnemer]

Ping.

[majnemer]

Ping.

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1580

thread_local local variables should presumably not be covered by this special case... though perhaps we never get here for those?

1596–1601

I think this comment should explain that we intend for the thread wrapper to be replaceable; otherwise, someone looking at this will wonder why we don't use the noinline attribute instead. Maybe isThreadLocalBacking... should instead be called isThreadWrapperReplaceable?

[majnemer]

• Address Richard's review comments.
• Address the latest review comments.

lib/CodeGen/ItaniumCXXABI.cpp

1580

I don't think that is possible, they should be completely handled by the `clang::CodeGen::CodeGenFunction::EmitStaticVarDecl` code path. I'll add an assert.

1596–1601

Done.

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1577

I don't think this checks the right thing: thread_local local variables have global storage, because they're implicitly static.

1580–1581

I think this logic is now revered compared to the function name.

1601

This line looks out of place, if we're going to be asserting that VD is not a static local.

[majnemer]

• Address review comments.

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1600–1602

I think you should also give a WeakODRLinkage variable a WeakAnyLinkage wrapper.

[majnemer]

• Address review comments.
• Give wrappers WeakAnyLinkage for WeakODRLinkage vars

[rsmith]

LGTM

lib/CodeGen/ItaniumCXXABI.cpp

1596–1601

Can you reword this? The reference to inlining doesn't make much sense here. I think we just want to say "If the thread wrapper is replaceable, give it appropriate linkage." -- but even this is kinda obvious from the code.

[majnemer]

Closed by commit rL212841 (authored by @majnemer).

lib/CodeGen/ItaniumCXXABI.cpp

void ItaniumCXXABI::registerGlobalDtor(CodeGenFunction &CGF,
   if (CGM.getLangOpts().AppleKext) {
     // Generate a global destructor entry.
     return CGM.AddCXXDtorEntry(dtor, addr);
   }
 
   CGF.registerGlobalDtorWithAtExit(D, dtor, addr);
 }
 
+static bool
+isThreadLocalBackingVariableAlwaysReferenceable(const VarDecl *VD,
+                                                CodeGen::CodeGenModule &CGM) {
+  // OS X prefers to have references to thread local variables to go through
+  // the thread wrapper instead of directly referencing the backing variable.
+  return VD->getTLSKind() != VarDecl::TLS_Dynamic ||
+         !CGM.getTarget().getTriple().isMacOSX();
+}
+
 /// Get the appropriate linkage for the wrapper function. This is essentially
 /// the weak form of the variable's linkage; every translation unit which wneeds
 /// the wrapper emits a copy, and we want the linker to merge them.
 static llvm::GlobalValue::LinkageTypes
 getThreadLocalWrapperLinkage(const VarDecl *VD, CodeGen::CodeGenModule &CGM) {
   llvm::GlobalValue::LinkageTypes VarLinkage =
       CGM.getLLVMLinkageVarDefinition(VD, /*isConstant=*/false);
 
   // For internal linkage variables, we don't need an external or weak wrapper.
   if (llvm::GlobalValue::isLocalLinkage(VarLinkage))
     return VarLinkage;
 
-  // All accesses to the thread_local variable go through the thread wrapper.
-  // However, this means that we cannot allow the thread wrapper to get inlined
-  // into any functions.
-  if (VD->getTLSKind() == VarDecl::TLS_Dynamic &&
-      CGM.getTarget().getTriple().isMacOSX())
-    return llvm::GlobalValue::WeakAnyLinkage;
+  // The thread wrapper can't be inlined if the backing variable cannot always
+  // be referenced.
+  if (!isThreadLocalBackingVariableAlwaysReferenceable(VD, CGM))
+    return VD->hasDefinition() ? llvm::GlobalValue::WeakAnyLinkage
+                               : llvm::GlobalValue::ExternalLinkage;
   return llvm::GlobalValue::WeakODRLinkage;
 }
 
 llvm::Function *
 ItaniumCXXABI::getOrCreateThreadLocalWrapper(const VarDecl *VD,
                                              llvm::GlobalVariable *Var) {
   // Mangle the name for the thread_local wrapper function.
   SmallString<256> WrapperName;
   {
     llvm::raw_svector_ostream Out(WrapperName);

 
 void ItaniumCXXABI::EmitThreadLocalInitFuncs(
     llvm::ArrayRef<std::pair<const VarDecl *, llvm::GlobalVariable *> > Decls,
     llvm::Function *InitFunc) {
   for (unsigned I = 0, N = Decls.size(); I != N; ++I) {
     const VarDecl *VD = Decls[I].first;
     llvm::GlobalVariable *Var = Decls[I].second;
 
+    // Some targets require that all access to thread local variables go through
+    // the thread wrapper.  This means that we cannot attempt to create a thread
+    // wrapper or a thread helper.
+    if (!isThreadLocalBackingVariableAlwaysReferenceable(VD, CGM) &&
+        !VD->hasDefinition())
+      continue;
+
     // Mangle the name for the thread_local initialization function.
     SmallString<256> InitFnName;
     {
       llvm::raw_svector_ostream Out(InitFnName);
       getMangleContext().mangleItaniumThreadLocalInit(VD, Out);
       Out.flush();
     }
 

test/CodeGenCXX/tls-init-funcs.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.8 -std=c++11 -S -emit-llvm %s -o - | FileCheck %s
 
 // CHECK: @a = internal thread_local global
 // CHECK: @_tlv_atexit({{.*}}@_ZN1AD1Ev
+// CHECK: call {{.*}} @_ZTW3ext()
+// CHECK: declare hidden {{.*}} @_ZTW3ext()
 // CHECK: define weak hidden {{.*}} @_ZTW1a
 
 struct A {
   ~A();
 };
 
 thread_local A a;
+
+extern thread_local A ext;
+A &get_ext() {
+  return ext;
+}