[libcxx-commits] [PATCH] D110349: [libcxx][SystemZ][z/OS] Added is_threading_api_enabled and might_have_multiple_threads to __threading_support

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi added inline comments.


================
Comment at: libcxx/include/__threading_support:293-298
+// On z/OS some posix functions can be enabled/disabled at runtime.
+_LIBCPP_THREAD_ABI_VISIBILITY
+bool __libcpp_are_threads_enabled();
+
+_LIBCPP_THREAD_ABI_VISIBILITY
+bool __libcpp_has_spawned_other_threads();
----------------
Quuxplusone wrote:
> DanielMcIntosh-IBM wrote:
> > Quuxplusone wrote:
> > > Peanut gallery says: It seems like it would be strictly easier to reason about this code if the user-facing API were called `bool __libcpp_might_have_multiple_threads()`. That is, IIUC, we don't care so much about what's "enabled" (potentia) so much as what is //the case// right now dynamically (actus).
> > > If `!__libcpp_might_have_multiple_threads()`, then we can skip locking mutexes (as long as they'll definitely be unlocked before the user gets a chance to spawn a second thread). I think that's what's going on in D113069, for example.
> > > Maybe this potentia/actus distinction is the same distinction as what you're calling `are_threads_enabled` (potentia) versus `has_spawned_other_threads` (actus); but if so, I haven't seen how you propose to use `has_spawned_other_threads`, and (FWLIW) I don't understand why D113069 used `are_threads_enabled` //instead// of `has_spawned_other_threads`.
> > > I say "might have multiple threads" instead of "definitely has multiple threads" because I imagine it's generally not possible for libc++ to tell whether the user might have spawned threads via pthreads or whatever — basically as you say on line 385, but making sure that the name of the function reflects its semantics.
> > > 
> > > (We had a bad time with `__libcpp_is_constant_evaluated`, where sometimes you wanted to distinguish between `__libcpp_might_be_constant_evaluated` and `__libcpp_is_definitely_runtime_evaluated`, and sometimes you wanted to distinguish between `__libcpp_is_definitely_constant_evaluated` and `__libcpp_might_be_runtime_evaluated`; and it was never obvious at the call-site which of those two interpretations `__libcpp_is_constant_evaluated` was meant to convey.) 
> > I have renamed them both to hopefully make things a bit more clear.
> > `are_threads_enabled` -> `is_threading_API_enabled`
> > `has_spawned_other_threads` -> `might_have_multiple_threads`
> > 
> > You're right that the potentia/actus distinction is the difference between `is_threading_api_enabled` and `might_have_multiple_threads`. However `is_threading_api_enabled` also tells us whether it is even //safe to try// locking a mutex, and unlike `might_have_multiple_threads`, it will not change over the lifetime of the process.
> > 
> > D113069 is kind of a bad example of when you would use one or the other, because (as per it's description), we //could probably// have used `might_have_multiple_threads`. The only reason we didn't was out of an abundance of caution.
> > For some better examples of when we would want to use `might_have_multiple_threads`, see D113058, D113066, D112567 and D110351. For an example of when we //have to// use `is_threading_api_enabled` instead of `might_have_multiple_threads`, see D113054
> > 
> Awesome. I know I'm bikeshedding here, but... now I understand (per the comment on line 341) that `not __libcpp_is_threading_api_enabled()` basically means `__libcpp_will_never_have_multiple_threads()`. ;)
> (However, I'm not fully happy with the word "never." We mean "this-specific-process-will-never-spawn-multiple-threads," but I can easily imagine someone misinterpreting it as "libcpp-policy-states-this-platform-will-be-single-threaded-forever." I don't know how to fix that problem. So I would leave your status quo until someone (maybe you) suggests anything strictly better than `__libcpp_is_threading_api_enabled`.)
> 
> IIUC, `__libcpp_might_have_multiple_threads` is for "do I need a lock+unlock around this global access, given that it's all happening before the next user code runs," and `__libcpp_is_threading_api_enabled` is for "do I need a lock before this access, given that some user code is going to run under the lock and thus `__libcpp_might_have_multiple_threads` may become true between now and the line that does the unlock."
> IIUC, `__libcpp_might_have_multiple_threads` is for "do I need a lock+unlock around this global access, given that it's all happening before the next user code runs," and `__libcpp_is_threading_api_enabled` is for "do I need a lock before this access, given that some user code is going to run under the lock and thus `__libcpp_might_have_multiple_threads` may become true between now and the line that does the unlock."

The `__libcpp_is_threading_api_enabled` checks if threading is enabled (POSIX ON) or not (POSIX OFF) as its name clearly indicates that. It cannot be changed throughout the live of the application and it is set at the compile time of the application. 
However, the `__libcpp_might_have_multiple_threads` can be changed in the life of the application. It is needed so the single threading execution can be made optimum. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110349/new/

https://reviews.llvm.org/D110349