This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add testing infrastructure for EXTERNAL_THREAD_API mode
AbandonedPublic

Authored by ldionne on Aug 27 2020, 5:40 AM.

Details

Reviewers
jroelofs
Nicu
miyuki
Group Reviewers
Restricted Project
Summary

Add example library to be able to test libcxx with EXTERNAL_THREAD_API mode.
Add cmake variable -DLIBCXX_TESTING_EXTERNAL_THREAD_API, and modify
testing configuration and build files, to be able to build libcxx in such mode.
Add cmake cache file for -DLIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY.

Co-authored-by: Mikhail Maltsev <mikhail.maltsev@arm.com>

Diff Detail

Event Timeline

Nicu created this revision.Aug 27 2020, 5:40 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptAug 27 2020, 5:40 AM
Nicu requested review of this revision.Aug 27 2020, 5:40 AM
ldionne requested changes to this revision.Aug 27 2020, 7:54 AM

I would like this and https://reviews.llvm.org/D86599 to be a single review, since I see those two as making the support for an external threading library official.

libcxx/CMakeLists.txt
262

I would like this test to be enabled unconditionally. We should always be testing it, and I don't want to add more logic to the CMake just to enable a test for another configuration.

Also, since this is just adding a test, we should be able to do so without touching the main CMakeLists.txt at all. The only code changes should be under libcxx/test, and the test should run through Lit like the other ones. I recognize this might be more work, but stringing the test of a given configuration by hacking around libc++'s build itself is just not a sustainable direction.

This revision now requires changes to proceed.Aug 27 2020, 7:54 AM

However, if that wasn't clear, I'm really happy we're making support for the external threading API official by adding tests -- otherwise I would have suggested dropping support for it as being untested. Thanks for working on that, I just think we need to nail down the proper way of doing it.

miyuki added inline comments.Aug 27 2020, 8:20 AM
libcxx/CMakeLists.txt
262

Normally enabling LIBCXX_HAS_EXTERNAL_THREAD_API requires the vendor to provide a version of __external_threading (e.g. like we do in the Arm Compiler toolchain) in the include directory. It is intended to be vendor-specific and unfortunately, it is needed at build time (not just during the test run).

LIBCXX_TESTING_EXTERNAL_THREAD_API makes the library use an example version of __external_threading from test/support/external-thread-api which relies on example_thread_library.c and its symbols. The intent is to check that the LIBCXX_HAS_EXTERNAL_THREAD_API mode works in principle.

So unfortunately I don't see a way to test the LIBCXX_HAS_EXTERNAL_THREAD_API configuration without modifications in the build logic of the library itself. @ldionne, what do you think?

Nicu updated this revision to Diff 289183.Sep 1 2020, 8:18 AM
Nicu edited the summary of this revision. (Show Details)

Update commit to include changes from https://reviews.llvm.org/D86599.
Update Summary to reflect this change.

Nicu added a comment.Sep 8 2020, 7:52 AM

I would like this and https://reviews.llvm.org/D86599 to be a single review, since I see those two as making the support for an external threading library official.

Merging of the both changes has been done.

@ldionne do you still think the change should be only modifying libcxx/test? As @miyuki mentioned, __external_threading is needed at build time so the main CMakeList.txt needs modification. In this case, the target cxx-external-threads which was previously working with LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY, has been used for LIBCXX_EXTERNAL_THREAD_API so that the build logic is similar.

In the case you still want these modifications to be enclosed only under libcxx/test, do you have any suggestions or ideas for such an approach?

miyuki commandeered this revision.Sep 29 2020, 10:57 AM
miyuki edited reviewers, added: Nicu; removed: miyuki.

@ldionne, do you have any thoughts on how we might progress this forward?

@ldionne, do you have any thoughts on how we might progress this forward?

Sorry for the radio silence -- I was working on something which should make this (and more) a lot easier to test.

I recently checked in CI configuration files that allow running tests under various configurations. I believe one way to test this external threading API would be to add a builder that tests this configuration. You can see the list of current configurations here: libcxx/utils/ci/run-buildbot.sh. Given this new setup where build bots are checked into the repo, I think adding a bot that test this configuration would be a good way of making sure this doesn't regress. What do you think?

Also, a meta question: Is there a reason why your platform can't simply provide the pthread or the C11 threading APIs, and libc++ has to provide this bridge instead? It would seem better for your platform to provide a standardized API instead, no? I'm trying to understand why the external threading API mode is useful (it feels like I've asked that before though, so just point me to the answer if that's the case).

Given this new setup where build bots are checked into the repo, I think adding a bot that test this configuration would be a good way of making sure this doesn't regress. What do you think?

Yes, setting up a build bot is part of my original plan. I will be on holiday for the next two weeks, but I do plan to continue working on this after returning.

Is there a reason why your platform can't simply provide the pthread or the C11 threading APIs, and libc++ has to provide this bridge instead?

The original intent of the external threading API was to make it usable with bare-metal real-time OS-es which tend not to have POSIX or POSIX-like APIs. For example, __libcpp_mutex_t can include the OS structure representing a mutex, instead of allocating it elsewhere.

Given this new setup where build bots are checked into the repo, I think adding a bot that test this configuration would be a good way of making sure this doesn't regress. What do you think?

Yes, setting up a build bot is part of my original plan. I will be on holiday for the next two weeks, but I do plan to continue working on this after returning.

Is there a reason why your platform can't simply provide the pthread or the C11 threading APIs, and libc++ has to provide this bridge instead?

The original intent of the external threading API was to make it usable with bare-metal real-time OS-es which tend not to have POSIX or POSIX-like APIs. For example, __libcpp_mutex_t can include the OS structure representing a mutex, instead of allocating it elsewhere.

Right -- my question is why is it not possible for these bare-metal real-time OS-es to implement these APIs? Or, alternatively, for whoever's trying to build libc++ on top of these systems to provide the right API as-if the system provided it. Doesn't this seem like a better solution than defining customization points at the libc++ level?

I'm not trying to push back -- this has been supported and we can continue supporting it, and I'm grateful for your desire to improve the test coverage in that area. However, I'm genuinely interested to find out whether/why this is the best solution.

miyuki added a comment.Oct 1 2020, 5:50 AM

Customization allows storing data of OS structures such as mutexes and threads in-place, thus avoiding the need for additional dynamic memory allocations, this is important in embedded environments.

ldionne commandeered this revision.Sep 13 2023, 2:45 PM
ldionne edited reviewers, added: miyuki; removed: ldionne.

[Github PR transition cleanup]

I'll abandon this because this wouldn't apply cleanly anymore, but most importantly because I think the future of supporting external threading configurations is not by keeping the external threading API the way it is today. I think it needs to be reworked to reduce the complexity it creates inside libc++ and I don't think it makes sense to add a testing configuration for it. Basically the way I envision this is that we'd move the "external threading API" from the current mechanism to a "vendor friendly" way of supporting different threading backends, like we do to define e.g. the different clocks or the different sources of randomness.

Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2023, 2:45 PM
Herald added a subscriber: arichardson. · View Herald Transcript
ldionne abandoned this revision.Sep 13 2023, 2:45 PM