Page MenuHomePhabricator

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

Authored by majnemer on Jun 12 2014, 2:17 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 10343.Jun 12 2014, 2:17 AM
majnemer retitled this revision from to CodeGen: Don't emit a thread-wrapper if we can't touch the backing variable.
majnemer updated this object.
majnemer added reviewers: rsmith, rjmccall.
majnemer added a subscriber: Unknown Object (MLST).
rsmith added inline comments.Jun 12 2014, 4:02 PM
lib/CodeGen/ItaniumCXXABI.cpp
1599 ↗(On Diff #10343)

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 ↗(On Diff #10343)

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

majnemer added inline comments.Jun 12 2014, 5:14 PM
lib/CodeGen/ItaniumCXXABI.cpp
1599 ↗(On Diff #10343)

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 ↗(On Diff #10343)

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 added inline comments.Jun 12 2014, 6:00 PM
lib/CodeGen/ItaniumCXXABI.cpp
1599 ↗(On Diff #10343)

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 ↗(On Diff #10343)

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 added inline comments.Jun 12 2014, 7:29 PM
lib/CodeGen/ItaniumCXXABI.cpp
1599 ↗(On Diff #10343)

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 ↗(On Diff #10343)

Then we can skip the hidden.

majnemer updated this revision to Diff 10377.Jun 12 2014, 8:27 PM
  • Address Richard's review comments.
rsmith added inline comments.Jun 27 2014, 8:39 AM
lib/CodeGen/ItaniumCXXABI.cpp
1580 ↗(On Diff #10377)

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

1596–1597 ↗(On Diff #10377)

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 updated this revision to Diff 10936.Jun 27 2014, 10:03 AM
  • Address Richard's review comments.
  • Address the latest review comments.
lib/CodeGen/ItaniumCXXABI.cpp
1580 ↗(On Diff #10377)

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–1597 ↗(On Diff #10377)

Done.

rsmith added inline comments.Jun 27 2014, 10:18 AM
lib/CodeGen/ItaniumCXXABI.cpp
1786 ↗(On Diff #10936)

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

1789–1790 ↗(On Diff #10936)

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

1810 ↗(On Diff #10936)

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

majnemer updated this revision to Diff 11133.Jul 7 2014, 4:55 PM
  • Address review comments.
rsmith added inline comments.Jul 8 2014, 6:00 PM
lib/CodeGen/ItaniumCXXABI.cpp
1846–1848 ↗(On Diff #11133)

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

majnemer updated this revision to Diff 11179.Jul 8 2014, 7:37 PM
  • Address review comments.
  • Give wrappers WeakAnyLinkage for WeakODRLinkage vars
rsmith accepted this revision.Jul 11 2014, 1:07 PM
rsmith edited edge metadata.

LGTM

lib/CodeGen/ItaniumCXXABI.cpp
1842–1844 ↗(On Diff #11179)

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.

This revision is now accepted and ready to land.Jul 11 2014, 1:07 PM
majnemer closed this revision.Jul 11 2014, 1:36 PM
majnemer updated this revision to Diff 11327.

Closed by commit rL212841 (authored by @majnemer).