This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] Add LIBCXXABI_HAS_WIN32_THREAD_API build option
ClosedPublic

Authored by zero9178 on Mar 5 2021, 1:56 AM.

Details

Summary

A few files in libc++abi make use of libc++ headers and a few of those use threading primitives provided by libc++. Since libc++ has multiple threading APIs it may be necessary to override auto-detection.

This patch adds the LIBCXXABI_HAS_WIN32_THREAD_API which does roughly the same as LIBCXXABI_HAS_PTHREAD_API and the similarly named LIBCXX_HAS_WIN32_THREAD_API from libc++. Instead of using autodetection it will force the use of win32 threads instead of pthreads in headers included from libc++.

Without this patch, libc++abi may depend on pthreads if present on the users build environment, even if win32 threading was selected for libc++.

Diff Detail

Event Timeline

zero9178 created this revision.Mar 5 2021, 1:56 AM
zero9178 requested review of this revision.Mar 5 2021, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 1:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo accepted this revision.Mar 5 2021, 2:05 AM

This LGTM (but I'm not a libcxx/libcxxabi approver, so you'll need to wait for the green checkmark for the libc++abi reviewer group), given the existing predecent with similar options for all other threading configurations. I've ran into this myself in various constellations, and in those cases worked around it by just passing -D_LIBCPP_HAS_THREAD_API_WIN32 in CMAKE_CXX_FLAGS, which is clearly a hack.

FWIW, there's work ongoing (see D97572) to make __config_site available directly which should avoid the issue if I remember correctly - and that would make these options in libcxxabi that mirror libcxx options redundant. But until that's in place, and since we already have these other ones, I think this looks sensible.

ldionne accepted this revision.Mar 5 2021, 6:26 AM

I'm really unhappy about this, but we do have LIBCXX_HAS_WIN32_THREAD_API so let's add this for consistency. When we find a way to replace these explicit settings for picking the threading API, we can remove them in both libc++ and libc++abi.

NB: The reason why I don't like this is that we normally never bake in this sort of detection in the CMake. I think it's a layering violation to do so. But like I said above, this is a pre-existing condition and this patch at least makes the layering violation consistent between libcxx and libcxxabi.

This revision is now accepted and ready to land.Mar 5 2021, 6:26 AM

Fully agree, making me even more excited for when the patch Martin mentioned lands.

This revision was landed with ongoing or failed builds.Mar 5 2021, 6:31 AM
This revision was automatically updated to reflect the committed changes.