Page MenuHomePhabricator

[libcxxabi] Refactor pthread usage into a separate API
ClosedPublic

Authored by rmaprath on Sep 23 2016, 7:18 AM.

Details

Summary

This is simply a cleanup of D18482 (patch taken with permission) while adapting it to match what we have already implemented for libcxx.

Note that I haven't included the external-threading stuff yet, this is only refactoring pthread usage of libcxxabi. A follow-up patch will add the external-threading support.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 72274.Sep 23 2016, 7:18 AM
rmaprath retitled this revision from to [libcxxabi] Refactor pthread usage into a separate API.
rmaprath updated this object.
rmaprath added a subscriber: cfe-commits.
rmaprath updated this revision to Diff 72478.Sep 26 2016, 7:19 AM

Use __internal_linkage__ attribute to control visibility following D24642.

compnerd added inline comments.Sep 26 2016, 11:20 AM
src/config.h
41

Can you change this to defined(__sun__) && defined(_POSIX_THREADS) at the very least? There is no reason to assume pthreads on Solaris. Solaris Threads are a valid threading model.

42

Can we use _LIBCXXABI_USE_THREAD_API_PTHREAD instead? You can have more than one threading API on a platform, and wish to use a specific one.

46

I really think that we should use _POSIX_THREADS macro rather than this enumeration approach.

test/test_fallback_malloc.pass.cpp
13

Can we add the include directory as a header search path for the tests? Seems it may be nicer, but, obviously, we can do that later.

EricWF edited edge metadata.Sep 26 2016, 4:31 PM

LGTM other than the inline comments. I'll give it a final once over tonight or tomorrow.

include/__cxxabi_config.h
36 ↗(On Diff #72478)

Does this configuration need to be in a public header if it's only used internally? I think config.h or threading_support.h would be a better place for this to live.

src/config.h
22

What's the point of defining _LIBCXXABI_WARNING? It's unused and seems unneeded.

src/fallback_malloc.ipp
30 ↗(On Diff #72478)

s/PTHREAD_MUTEX_INITIALIZER/_LIBCXXABI_MUTEX_INITIALIZER

rmaprath updated this revision to Diff 72653.Sep 27 2016, 7:57 AM
rmaprath edited edge metadata.

Address review comments from @compnerd and @EricWF.

rmaprath added inline comments.Sep 27 2016, 7:58 AM
src/config.h
22

Good catch. I have copy-pasted this lot without checking.

41

Not sure if I understand completely, libcxxabi currently either supports pthread or no-threads, either of which can be explicitly configured through a cmake option. So I don't see the point in checking for _POSIX_THREADS here as the only current threading option is pthread. When we add another threading system into the mix (like external threading), that too will be configurable with a cmake option, the defaulting to pthread is only when no particular threading system is selected.

Or is it that pthread.h et. al can be provided by Solaris Threads as well? So, the pthread API has two implementations on Solaris?

42

Makes sense. Need to change the convention used in libcxx too, but that can be done later.

46

_LIBCXXABI_USE_THREAD_API_XXXX allows us to be consistent with naming of the different threading variants (e.g. I'm going to add _LIBCXXABI_USE_THREAD_API_EXTENRAL soon). Is there some functional benefit to using _POSIX_THREADS for the pthread variant?

src/fallback_malloc.ipp
30 ↗(On Diff #72653)

Thanks for the catch.

EricWF accepted this revision.Sep 28 2016, 2:44 PM
EricWF edited edge metadata.

LGTM once the _POSIX_THREADS comment is addressed.

src/config.h
46

I think what @compnerd means is that _POSIX_THREADS can be used to detect if the current platform supports/provides pthreads.h. So instead of enumerating the OS's that support them manually we can just replace that check with defined(_POSIX_THREADS) after including unistd.h.

This revision is now accepted and ready to land.Sep 28 2016, 2:44 PM
rmaprath updated this revision to Diff 73007.Sep 30 2016, 1:59 AM
rmaprath edited edge metadata.

Updated with the following changes:

  • Address review comments from @EricWF and @compnerd regarding using _POSIX_THREADS for detecting pthread availability. Now the patch is checking for defined(_POSIX_THREADS) && _POSIX_THREADS >= 0 instead of enumerating the platforms.
  • Fix a missed pthread dependency in cxa_guard.cpp. The call to pthread_mach_thread_np is now replaced with __libcxxabi_thread_get_current_id().
  • Fix a couple of missing pthread dependencies in test_exception_storage.cpp (see patch below).

@EricWF: Hope the new changes look OK? I'm going to test this on a Mac before committing.

I will also ping D21803 and D17815, as they'll be affected by this patch (those patches add more pthread calls).

I'm going to test this on a Mac before committing.

Good call that was, wouldn't have compiled there. Attaching updated patch soon.

rmaprath updated this revision to Diff 73033.Sep 30 2016, 4:24 AM

pthread_mach_thread_np is Mac specific. I've introduced a Mac-only __libcxxabi_thread_get_port() API call to sort this out (didn't think it would be nice to leave a pthread call even if it is Mac specific).

I can revisit this if there is a better approach. Will commit now.

Sigh. I've bumped into some downstream problems. Will commit as soon as I've verified these problems are not related to the patch.

rmaprath updated this revision to Diff 74519.Oct 13 2016, 8:09 AM

Patch re-based on the latest trunk.

I've resolved my downstream issues, will be committing soon.

/ Asiri

rmaprath closed this revision.Oct 14 2016, 2:13 AM

Committed as r284128.

So there is:

  • Looking for __cxa_thread_atexit_impl in c
  • Looking for __cxa_thread_atexit_impl in c - not found

and libcxx is configured with -DLIBCXX_ENABLE_THREADS=OFF

rmaprath added a comment.EditedOct 14 2016, 4:51 PM

So there is:

  • Looking for __cxa_thread_atexit_impl in c
  • Looking for __cxa_thread_atexit_impl in c - not found

and libcxx is configured with -DLIBCXX_ENABLE_THREADS=OFF

I think, the problem here is that cxa_thread_atexit.cpp is not properly guarded against the non-threaded use-case. If you look at the source prior to this patch, it refers to pthread.h and pthread_key_t unconditionally, it may have worked before because pthread got somehow dragged in.

The fix should be fairly straightforward. I will do a patch tomorrow (bit late in the day here), hope that's OK?

Cheers,

/ Asiri

Maybe?

  • if (UNIX AND NOT (APPLE OR CYGWIN))

+ if (LIBCXXABI_ENABLE_THREADS AND UNIX AND NOT (APPLE OR CYGWIN))

list(APPEND LIBCXXABI_SOURCES cxa_thread_atexit.cpp)

endif()

Maybe?

- if (UNIX AND NOT (APPLE OR CYGWIN))
+ if (LIBCXXABI_ENABLE_THREADS AND UNIX AND NOT (APPLE OR CYGWIN))
list(APPEND LIBCXXABI_SOURCES cxa_thread_atexit.cpp)
endif()

Yes!

I was just about to say I don't know why it's even trying to build this, given that the library is configured with -DLIBCXXABI_ENABLE_THREADS=OFF.

Feel free to commit a fix :)

Cheers,

/ Asiri

Thanks, done D25636

Thanks for the fix!

/ Asiri