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

Mon Mar 14 11:25:27 PDT 2022

DanielMcIntosh-IBM added a comment.

In D120348#3375321 <https://reviews.llvm.org/D120348#3375321>, @zibi wrote:

> 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.

Just to clarify this point a little bit: There are many places where we could use either `might_have_multiple_threads()` or `is_threading_api_enabled()`. However, some places are a little more sensitive to which one we choose to use.

First, `__cxa_get_globals_fast()` needs to be as consistent as possible about which `__cxa_eh_globals` object it associates with a given thread. This makes `might_have_multiple_threads()` a bad choice here, and using it would effectively forbid users from spawning threads while exception handling is ongoing. Spawning a thread inside a catch block seems like uncommon, but not unreasonable behaviour, so we'd like to support it if possible. To a lesser extent, this is also true for spawning a thread in a deconstructor during stack unwinding. Banning users from loading the thread library under the same circumstances is much more palatable, not to mention that loading the thread library mid-program is not possible on platforms like z/OS anyways.

Second, cxa_guard cannot invoke any functions which use static local variables. So, the functions it uses when locking/unlocking a mutex don't really have any decent way of preserving information across invocations. In other words, even though `is_threading_api_enabled()` doesn't change during the application lifetime, if cxa_guard was to use it, we'd have to get rid of any static locals from it, and re-load the value from `__libcpp_ceeedb_posix()` every time. We could create a separate `is_threading_api_enabled_no_statics()`, `internal_thread::__libcpp_mutex_lock_no_static()`, `internal_thread::__libcpp_condvar_broadcast_no_static()`, etc. but at that point why not just use `might_have_multiple_threads()`?
Admittedly, we could remove any static locals from `is_threading_api_enabled()` and just tank the performance hit that comes with making something like 5 sequential load operations every time we call it (2 in `__libcpp_ceeedb_flags` and 3 when that calls `__gtca()`), but as you pointed out, some of these areas appear to be performance critical (e.g. `__cxa_get_globals_fast()`).

================
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() {
----------------
EricWF wrote:
> `s/should not/does not` Because that's necessary for correctness. 
> When exactly at runtime does this property become fixed?
> When exactly at runtime does this property become fixed?

On z/OS, it's set by the OS prior to launching an application (that is, prior to ANYTHING that the user/library has any control over)

================
Comment at: libcxx/src/include/internal_threading_support.h:67
+    return 0;
+  return _VSTD::__libcpp_recursive_mutex_init(__m);
+}
----------------
zibi wrote:
> 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.
The names were chosen to match the names from `__threading_support` since these are mostly just thin wrappers around those functions and intended as drop-in replacements. I don't see any reason we couldn't use different names, but it should be clear which function from `__threading_support` each one is a wrapper for. Since they're supposed to be drop-in replacements the most logical thing seemed to be to use the same names, but we could add a `_wrapped` suffix or something I suppose.

================
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;
----------------
zibi wrote:
> 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.
As zibi said, it can change during the execution of the program, but it can't spontaneously go from false to true. That would require e.g. spawning a thread.

================
Comment at: libcxx/src/include/internal_threading_support.h:76
+}
+
+static inline bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t* __m) {
----------------
zibi wrote:
> 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.
z/OS has very limited and weak support (no pun intended) for weak linking. It really only works if both definitions are visible at link time, and neither is coming from a shared library.

During the link stage, z/OS will prioritize symbols from object/source files and such over shared libraries, regardless of whether they've been declared as weak definitions. Furthermore, when searching for a symbol in shared libraries, z/OS uses "Side decks" to determine which shared libraries provide which symbols. Side decks do not record information about whether a symbol is weak, so it's impossible to weakly export a symbol from a shared library.

================
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
----------------
EricWF wrote:
> 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`.
Many of these are not called as far as I'm aware. They're just provided for increased parity with `__threading_support` (though I'm noticing now that we have already dropped some functions). We might be able to remove many of them if you'd prefer a more limited set of definitions here.

================
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.
----------------
zibi wrote:
> 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.
This comment is a little misleading now that this bit of code is z/OS-specific. Initially we'd hoped to write this in a more generic way, but on some platforms `__libcpp_exec_once_flag` is a struct, which made everything too complicated. We should probably update the comment to make the intent more clear. It was simply intended as a warning for other platforms like AIX where `__libcpp_is_threading_api_enabled()` might change mid-program that they might not be able to re-use this because of the `*flag = 2;` part.

Side note: @zibi It won't make any difference to program correctness, but now that this code is z/OS specific it might be a good idea to make sure that we're writing the same value `_VSTD::__libcpp_execute_once` would write.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120348