Page MenuHomePhabricator

[libcxxabi] Introduce an externally threaded libc++abi variant
AbandonedPublic

Authored by rmaprath on Nov 29 2016, 5:11 AM.

Details

Summary

This is more-or-less a mirror image of D21968, repeated for libcxxabi.

I will soon upload a dependent patch for libcxx which makes it possible to run the libcxx test suite with the externally-threaded libcxxabi variant.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 79544.Nov 29 2016, 5:11 AM
rmaprath retitled this revision from to [libcxxabi] Introduce an externally threaded libc++abi variant.
rmaprath updated this object.
rmaprath added reviewers: mclow.lists, EricWF, bcraig.
rmaprath added a subscriber: cfe-commits.
EricWF edited edge metadata.Dec 5 2016, 1:08 AM

My main issue with this patch (and D27206) is that there are now two different CMake options for building two different external threading libraries. I would much prefer having libc++abi use libc++'s __threading_support header and cxx_external_threads library.

CMakeLists.txt
124

It's weird that libc++abi needs to define a libc++ option? Can you elaborate on the need for this?

src/CMakeLists.txt
132

Do you really need to glob a single file?

test/CMakeLists.txt
41

Target names shouldn't be in quotes.

45

For the in-tree libc++/libc++abi builds libc++abi gets configured before libc++, so I don't think libc++ target names are visible in this context.
Are you sure this works?

test/libcxxabi/test/config.py
43

libcxxabi_shared should default to True like it does within the libc++ config.

test/lit.cfg
21

I'm not sure I understand the reason for this change.

My main issue with this patch (and D27206) is that there are now two different CMake options for building two different external threading libraries. I would much prefer having libc++abi use libc++'s __threading_support header and cxx_external_threads library.

I would like to do that too!

But I always viewed libcxx and libcxxabi as two independent libraries that should be usable on their own. In this case, binding libcxxabi to __threading_support from libcxx sounded wrong, because __threading_support is something internal to libcxx (e.g. all the functions there are prefixed with __libcpp_ - there may be other libcpp-isms arising from __config in there too).

Is that not a concern? I could give it a shot and try to merge the two APIs into one if not.

EricWF added a comment.Dec 5 2016, 1:53 AM

My main issue with this patch (and D27206) is that there are now two different CMake options for building two different external threading libraries. I would much prefer having libc++abi use libc++'s __threading_support header and cxx_external_threads library.

I would like to do that too!

But I always viewed libcxx and libcxxabi as two independent libraries that should be usable on their own. In this case, binding libcxxabi to __threading_support from libcxx sounded wrong, because __threading_support is something internal to libcxx (e.g. all the functions there are prefixed with __libcpp_ - there may be other libcpp-isms arising from __config in there too).

Ironically I've always viewed libcxxabi as fully dependent on libcxx (and personally I would like to see the repos merged; Although using libc++ w/o libc++abi would still be supported).

Is that not a concern? I could give it a shot and try to merge the two APIs into one if not.

It's not a concern. Libc++abi already depends on libc++ internals to build (See the #include "__refstring" in stdlib_stdexcept.cpp`).

My main issue with this patch (and D27206) is that there are now two different CMake options for building two different external threading libraries. I would much prefer having libc++abi use libc++'s __threading_support header and cxx_external_threads library.

I would like to do that too!

But I always viewed libcxx and libcxxabi as two independent libraries that should be usable on their own. In this case, binding libcxxabi to __threading_support from libcxx sounded wrong, because __threading_support is something internal to libcxx (e.g. all the functions there are prefixed with __libcpp_ - there may be other libcpp-isms arising from __config in there too).

Ironically I've always viewed libcxxabi as fully dependent on libcxx (and personally I would like to see the repos merged; Although using libc++ w/o libc++abi would still be supported).

Is that not a concern? I could give it a shot and try to merge the two APIs into one if not.

It's not a concern. Libc++abi already depends on libc++ internals to build (See the #include "__refstring" in stdlib_stdexcept.cpp`).

Thanks for the clarification. I'll re-spin the patch.

Responses to inline comments follow, but some of those would go away with the re-spin I imagine.

CMakeLists.txt
124

The libcxxabi test suite builds libcxx as part of running the test suite, I thought a separate option would be appropriate here to allow users to select which configuration they want to build libcxx for those tests.

Given the new knowledge I have, I think the right thing to do here is configure libcxx to also use external threads when libcxxabi is configured to use external threads, rather than providing a separate option here. Will fix.

src/CMakeLists.txt
132

Copy-paste, will fix.

test/CMakeLists.txt
41

Will fix.

45

This worked fine for me. All my builds are in-tree as well.

I also checked if the order in which tests are run (whether libcxx runs before libcxxabi and vice-versa) makes no difference.

I will double check still.

test/libcxxabi/test/config.py
43

Not sure if I follow, I'm trying to detect this particular combination of configs so that I can xfail a particular test.

test/lit.cfg
21

I should've put a comment. The test suite keeps trying to run test/support/external_threading.cpp as a test case without this. Perhaps there is a better way to handle that situation?