This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][SystemZ][z/OS] Update libcxx/src/random_shuffle.cpp to accommodate POSIX(OFF)
AbandonedPublic

Authored by DanielMcIntosh-IBM on Nov 2 2021, 4:23 PM.

Details

Reviewers
jroelofs
EricWF
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Summary

Check that the threading API is enabled in the __rs_default constructor/
destructor in order to prevent the 2 argument form of std::random_shuffle from
calling mutex functions when they're disabled.

We might be able to use __libcpp_has_spawned_other_threads instead of
__libcpp_are_threads_enabled here, but there is a large amount of code that
runs between the mutex lock and unlock. Assuming that we never encounter a
situation where __rs_default::__rs_default() is run twice (once single-threaded
and once multi-threaded) before we run __rs_default::~__rs_default() once is
therefore a risky proposition.

Depends on D110349

Diff Detail

Event Timeline

DanielMcIntosh-IBM requested review of this revision.Nov 2 2021, 4:23 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 4:23 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jroelofs added inline comments.Nov 3 2021, 11:25 AM
libcxx/src/random_shuffle.cpp
28

should this guard go inside __libcpp_mutex_{,un}lock instead?

Quuxplusone added inline comments.Nov 3 2021, 1:52 PM
libcxx/src/random_shuffle.cpp
28

(This function comes from parent revision D110349)
FWLIW, I think yes it should.

DanielMcIntosh-IBM edited the summary of this revision. (Show Details)Nov 4 2021, 3:51 PM

Update function names as a result of changes to D110349

libcxx/src/random_shuffle.cpp
28

I had considered doing that, but don't think it's a good idea because

  1. it would result in a small performance hit in a lot of places
  2. since cxa_guard (which manages initialization of static local variables) uses __libcpp_mutex_{,un}lock, it would cause infinite recursion (__libcpp_mutex_{,un}lock -> __libcpp_is_threading_api_enabled() -> cxa_guard -> __libcpp_mutex_{,un}lock). This loop could be broken by getting rid of the static local variable in __libcpp_is_threading_api_enabled(), but I think that is a significantly worse option. Also, getting rid of the static local variable makes the performance hit in 1) a lot bigger
Quuxplusone added inline comments.Nov 5 2021, 9:37 AM
libcxx/src/random_shuffle.cpp
28

Again FWLIW: Makes sense to me. :)
I briefly wondered if constinit could help with the initialization in D110349, but no, it can't. Although... maybe you could use atomic? or is that a case of "now you have two problems"? ;) https://godbolt.org/z/objbvvdx3 In the thread-safe-static-initialization case, notice that declaring __gtca() as noexcept improves codegen, because then Clang doesn't have to codegen a "failure" path for the initialization.

DanielMcIntosh-IBM marked 3 inline comments as done.Nov 8 2021, 1:03 PM
DanielMcIntosh-IBM added inline comments.
libcxx/src/random_shuffle.cpp
28

The way __gtca() is currently defined/declared in __gfunc.h, Clang won't codegen the failure path anyways.

As for atomic, while it might work, I'm not convinced it would be any better performance wise than a static local variable, and it's definitely not easier to read.

However, using atomic in __libcpp_ceeedb_flags() might provide a significant performance benefit to might_have_multiple_threads. CEECAA, CEECAAEDB, and CEEEDBFLAG1 won't change over the lifetime of the process, but since cxa_guard uses might_have_multiple_threads (see D110351), none of them can be static local variables. If using atomic there does provide a performance benefit, then it might be worth considering, but that can happen at a later date.

DanielMcIntosh-IBM marked an inline comment as done.

Rebase and update indentation to match the rest of the file