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

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added a comment.

Note that although I'm picking nits, I'm not an approver for this. I assume you'll want to get ldionne's attention on this entire patch series.



================
Comment at: libcxx/include/__support/ibm/ceeedb.h:46
+  const unsigned char CEEEDB_POSIX = '\x04';
+  return __libcpp_ceeedb_flags() & CEEEDB_POSIX;
+}
----------------
Nits: Surely `'\x04'` is more confusing than `4`?
Should you guard against the user doing `#define CEEEDB_POSIX 42` before including this header? Ditto for all non-reserved identifiers used in this file (lines 22–26, 32–36, 40, 45).


================
Comment at: libcxx/include/__threading_support:343-344
+bool __libcpp_is_threading_api_enabled() {
+  static bool posix_on = __libcpp_ceeedb_posix();
+  return posix_on;
+}
----------------
`s/posix_on/__posix_on/`


================
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();
----------------
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."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110349