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

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi marked 19 inline comments as done.
zibi added a comment.

The `__libcpp_is_threading_api_enabled()` checks if threading is enabled (POSIX ON) or not (POSIX OFF) as its name clearly indicates. It cannot be changed throughout the 
live of the application but it can be set at the start of the application via env. var. `_CEE_RUNOPTS=POSIX(OFF)`.

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.



================
Comment at: libcxx/src/include/internal_threading_support.h:30
+#  if defined(__MVS__)
+static inline bool __libcpp_is_threading_api_enabled();
+
----------------
DanielMcIntosh-IBM wrote:
> EricWF wrote:
> > Why `static inline`? 
> To prevent this symbol from getting exported as part of the library (it shouldn't ever get invoked by user code and is purely for internal use within libcxx/libcxxabi source), and also to prevent duplicate definitions as a result of using this header in both `libcxx` and `libcxxabi`.
The static inline was chosen so they are optimized out on the platforms which do not have dynamic threading support.


================
Comment at: libcxx/src/include/internal_threading_support.h:67
+    return 0;
+  return _VSTD::__libcpp_recursive_mutex_init(__m);
+}
----------------
EricWF wrote:
> 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?
Are you suggesting to change names for all `__libcpp_recursive_foo()` functions in this file and `__threading_support`? We use static mechanism and 
@DanielMcIntosh-IBM  can provide more details if needed.


================
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;
----------------
EricWF wrote:
> Can `__libcpp_might_have_multiple_threads()` change during program execution? Or is its value fixed after program load?
It can change during the execution of the program.


================
Comment at: libcxx/src/include/internal_threading_support.h:76
+}
+
+static inline bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t* __m) {
----------------
EricWF wrote:
> Why not just use weak functions to allow different definitions, which can live outside of mainline libc++, to be shimmed in at link/load time? 
We will have to experiment to see if that is even fisible.


================
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())
----------------
EricWF wrote:
> 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; }
> ```
The `__libcpp_might_have_multiple_threads` can change during the execution.


================
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.
----------------
EricWF wrote:
> 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,
The value of `__libcpp_is_threading_api_enabled()` can only change at the load time.


================
Comment at: libcxx/src/include/internal_threading_support.h:178
+    // __libcpp_is_threading_api_enabled() to return false.
+    assert(false);
+#  endif
----------------
EricWF wrote:
> No `assert`. Use one of the abort methods. 
Can you provide an example for it?


================
Comment at: libcxx/src/locale.cpp:730
 {
+#ifndef _LIBCPP_HAS_NO_THREADS
+    if (!__libcpp_is_threading_api_enabled()) {
----------------
DanielMcIntosh-IBM wrote:
> EricWF wrote:
> > 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?
> > 
> `call_once` doesn't need to work when you threads have been disabled at runtime. As I discuss in D117372 it's actually kind of foolish to use call_once when threads have been disabled.
> 
> We could still support it, but we'd have to make changes to `std::__call_once`. That is an option, and it's what I went with in V1 and V2, but as I discuss in D117372, I think the better option is to eventually stop using `call_once` or any other parts of the C++11 Thread Support Library in the rest of libc++.
Will have a look at that. @DanielMcIntosh-IBM are then any plans to make changes to `call_once()` in future patches?


================
Comment at: libcxxabi/src/cxa_exception_storage.cpp:96
+        // If threads are disabled at runtime, revert to single-threaded implementation.
+        if (!__libcpp_is_threading_api_enabled()) {
+            static __cxa_eh_globals eh_globals;
----------------
DanielMcIntosh-IBM wrote:
> EricWF wrote:
> > 
> > This seems like performance sensitive code. Are we cool with a branch here?
> > 
> > Also, are we even allowed to use function local statics in this function? 
> > 
> On platforms without runtime-dependent threading, `__libcpp_is_threading_api_enabled()` is constexpr, so this should almost always be optimized out (and every compiler I can find will do it even on `-O0`).
> 
> I don't see why we couldn't use function local statics here? We use them when `HAS_THREAD_LOCAL` is defined (though they are `thread_local` there).
I think so, this is done in single threaded case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120348