This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] Add __cxa_thread_atexit for TLS support on Linux.
ClosedPublic

Authored by danalbert on Dec 17 2014, 10:37 AM.

Diff Detail

Event Timeline

danalbert updated this revision to Diff 17405.Dec 17 2014, 10:37 AM
danalbert retitled this revision from to [libcxxabi] Add __cxa_thread_atexit for TLS support on Linux..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: mclow.lists, EricWF, jroelofs.
danalbert added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
src/cxa_thread_atexit.cpp
18–20

The only libc which provides __cxa_thread_atexit_impl is glibc. Other libc implementations on linux do not and it doesn't seem available on the BSDs.

I think it makes sense to forward to __cxa_thread_atexit_impl if the symbol exists, otherwise we need to have a definition of __cxa_thread_atexit which can do the real heavy lifting.

jroelofs added inline comments.Dec 17 2014, 11:57 AM
test/cxa_thread_atexit_test.cpp
15

What about when the libc provides this symbol? Is it normally weak?

16

to have the same signature, this needs throw().

danalbert added inline comments.Dec 17 2014, 12:17 PM
src/cxa_thread_atexit.cpp
18–20

Oops. I added a guard to the header to make sure this was only there on Linux but forgot to do the same here and in the test.

BTW, glibc is the only one that supports it for now, but we're working on adding it to Bionic.

As for non-glibc libc implementations for non-Android Linux, are you thinking about newlib/musl? Those implementations would just need to add support for this if they want this kind of TLS; I don't think we need to add it here.

test/cxa_thread_atexit_test.cpp
15

The dynamic linker satisfies the symbol from the main executable first, even if there is a strong symbol in another shared object.

16

This is the _impl function, which doesn't have the throw() specifier (since it's C code). I suppose the throw() on the non-impl version of this is actually unnecessary since it's in extern "C". I had just match the Apple functions above my decl in cxxabi.h.

danalbert updated this revision to Diff 17411.Dec 17 2014, 2:00 PM

Make the implementation only build on Linux, and make the test require linux.

majnemer added inline comments.Dec 17 2014, 2:13 PM
src/cxa_thread_atexit.cpp
18–20

musl does not which makes me feel like we shouldn't unconditionally try to provide it.

I think it would make more sense to make it conditional on _GLIBCXX_HAVE___CXA_THREAD_ATEXIT_IMPL. Bionic can create its own way to signal that it has the function.

danalbert updated this revision to Diff 17413.Dec 17 2014, 2:57 PM

Only build the implementation for cxa_thread_atexit if libc has
cxa_thread_atexit_impl.

test/cxa_thread_atexit_test.cpp
16

jroelofs: Hm. What about with statically linked libc?

I was under the impression that static linking on Linux wasn't really a thing any more: http://stackoverflow.com/questions/3430400/linux-static-linking-is-dead

At any rate, our tests will not build statically on Linux, so it's a non-issue. It's probably broken, but if we ever get to a state where we care about building this test statically, I'll fix it up then.

jroelofs added inline comments.Dec 17 2014, 3:01 PM
test/cxa_thread_atexit_test.cpp
16

sounds good.

jroelofs accepted this revision.Dec 17 2014, 3:59 PM
jroelofs edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 17 2014, 3:59 PM
danalbert closed this revision.Dec 17 2014, 4:04 PM