[PATCH] D110349: [libcxx][SystemZ][z/OS] Added are_threads_enabled and has_spawned_other_threads to __threading_support

[Daniel McIntosh via Phabricator via llvm-commits]

DanielMcIntosh-IBM 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:
> 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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110349