[libcxx-commits] [PATCH] D120348: [libcxx][SystemZ][ POSIX(OFF) support on z/OS

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

I made a few mistakes in my first set of comments. I better understand now. But I'm even more concerned.
I'll spend some time reading the design doc to see if that answers my quenstions.



================
Comment at: libcxx/src/include/internal_threading_support.h:27
+#ifndef _LIBCPP_HAS_NO_THREADS
+#  include <cassert>
+
----------------
DanielMcIntosh-IBM wrote:
> EricWF wrote:
> > I think we're very trying to avoid including `<cassert>` anywhere within libc++ headers.
> Even for internal headers? (Note that this file is under `libcxx/src/include` rather than `libcxx/include`)
Sorry, I see now that this isn't strictly a libc++ header.


================
Comment at: libcxx/src/include/internal_threading_support.h:43
+/// On z/OS some posix functions can be enabled/disabled at runtime.
+/// However, the enabled/disabled status should not change over the life of the process.
+bool __libcpp_is_threading_api_enabled() {
----------------
`s/should not/does not` Because that's necessary for correctness. 
When exactly at runtime does this property become fixed?


================
Comment at: libcxx/src/include/internal_threading_support.h:62
+_LIBCPP_BEGIN_NAMESPACE_STD
+namespace internal_thread {
+
----------------
EricWF wrote:
> This would need to be a reserved identifier.
Disregard that comment. I see I was mistakne.


================
Comment at: libcxx/src/include/internal_threading_support.h:67
+    return 0;
+  return _VSTD::__libcpp_recursive_mutex_init(__m);
+}
----------------
This non-recursive call that looks super recursive makes me super uncomfortable.
Can't we just choose a different name?

What does the call to `_VSTD::__libcpp_recursive_mutex_init` do if threading is fully disabled?


================
Comment at: libcxx/src/include/internal_threading_support.h:72
+  // See __libcpp_mutex_lock for an explaination of why this is safe
+  if (!__libcpp_might_have_multiple_threads())
+    return 0;
----------------
Can `__libcpp_might_have_multiple_threads()` change during program execution? Or is its value fixed after program load?


================
Comment at: libcxx/src/include/internal_threading_support.h:84
+
+static inline int __libcpp_recursive_mutex_unlock(__libcpp_recursive_mutex_t* __m) {
+  // See __libcpp_mutex_lock for an explaination of why this is safe
----------------
When are any of these functions called?

For example, there's nothing in the `src/` directory that calls `__libcpp_thread_yield()` but there's a definition for `internal_thread::__libcpp_thread_yield`.


================
Comment at: libcxx/src/include/internal_threading_support.h:91
+
+static inline int __libcpp_recursive_mutex_destroy(__libcpp_recursive_mutex_t* __m) {
+  if (!__libcpp_is_threading_api_enabled())
----------------
If `__libcpp_might_have_multiple_threads` is a property set during program load. Couldn't these simply be

```
static auto  __libcpp_foo = __libcpp_might_have_multiple_threads() ? +[](auto args...) { return std::__libcpp_foo(args...); } : +[](auto args...) { return 0; }
```


================
Comment at: libcxx/src/include/internal_threading_support.h:170
+      init_routine();
+      // TODO: In order for this to work when __libcpp_is_threading_api_enabled() can change during
+      // program execution, we have to write the same value pthread_once would.
----------------
I thought the value of `__libcpp_is_threading_api_enabled` cannot change during program execution.

I don't see how any of this code is correct if that property can change. What if it changes right after our call to `__libcpp_is_threading_api_enabled()` ? Then whatever we do next is racey because we're skipping the now required synchronization,


================
Comment at: libcxx/src/locale.cpp:730
 {
+#ifndef _LIBCPP_HAS_NO_THREADS
+    if (!__libcpp_is_threading_api_enabled()) {
----------------
I would prefer we just use `call_once` in all cases. Why can't we do that?
Presumably `call_once` needs to work when you have runtime enable/disabled threads?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120348