This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vlad.tsyrklevich on Jan 16 2019, 3:27 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

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 added inline comments.Jan 16 2019, 9:30 PM
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 marked an inline comment as done.Jan 16 2019, 9:37 PM
vlad.tsyrklevich added inline comments.
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 marked an inline comment as done.Jan 16 2019, 9:50 PM
vlad.tsyrklevich added inline comments.
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 accepted this revision.Jan 16 2019, 10:31 PM

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.

This revision is now accepted and ready to land.Jan 16 2019, 10:31 PM
This revision was automatically updated to reflect the committed changes.