This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][SystemZ][ POSIX(OFF) support on z/OS
Needs ReviewPublic

Authored by zibi on Feb 22 2022, 12:34 PM.

Details

Reviewers
ldionne
Quuxplusone
DanielMcIntosh-IBM
SeanP
EricWF
Group Reviewers
Restricted Project
Restricted Project
Summary

This is the 3rd version for POSIX(OFF) on z/OS. It replaces previous proposals in D110349 and D117375. This patch will help out other platforms where threading support can be determined and/or switched at run-time like AIX.

The full support on z/OS for POSIX(OFF) depends on the following, additional patches from version 2 (D117375). Eventually they need to be landed as well. I'm listing them just for reference:

D117373
D117372
D117366

Please be aware that this revision doesn't include the necessary work for:

  1. the shared_ptr overloads of std::atomic_foo()
  2. Debug Mode
  3. std::random_shuffle(first, last)
  4. fallback_malloc

Diff Detail

Event Timeline

zibi created this revision.Feb 22 2022, 12:34 PM
zibi requested review of this revision.Feb 22 2022, 12:34 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 22 2022, 12:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zibi edited the summary of this revision. (Show Details)Feb 22 2022, 12:36 PM

Does this mean that D110349 and/or D117375 could be closed/abandoned at this point?

libcxxabi/src/cxa_guard_impl.h
296

Do you know a concrete reason that _VSTD is needed here, i.e. why wouldn't std::internal_thread::__libcpp_condvar_broadcast work here?

zibi marked an inline comment as done.Feb 22 2022, 5:44 PM

Does this mean that D110349 and/or D117375 could be closed/abandoned at this point?

Yes, they will be closed once this is approved.

libcxxabi/src/cxa_guard_impl.h
296

No reason, I will change it to std::internal_thread::

zibi updated this revision to Diff 410689.Feb 22 2022, 6:07 PM
zibi marked an inline comment as done.

Change _VSTD to std and formatting changes to fix CI.

zibi updated this revision to Diff 410945.Feb 23 2022, 2:30 PM

Formatting and fixing MAC/WIN/AIX builds.

zibi updated this revision to Diff 410975.Feb 23 2022, 5:02 PM

Rebase to fix CI documentation error

DanielMcIntosh-IBM edited the summary of this revision. (Show Details)Feb 24 2022, 12:21 PM
DanielMcIntosh-IBM added inline comments.
libcxx/src/include/internal_threading_support.h
55–59

This is redundant because it's part of the forward declaration.

71

Unlike _VSTD::__libcpp_foo, these are never provided by an external library, nor are the definitions ever used to build an external library, so they should probably always be inline and hidden from the ABI.

198
199
libcxx/src/include/internal_threading_support.h
71

This will also probably fix the Windows CI failures.

zibi updated this revision to Diff 411296.Feb 24 2022, 7:45 PM

Attempt to fix CI with _LIBCXXABI_HAS_NO_THREADS guard.

zibi updated this revision to Diff 411310.Feb 24 2022, 9:31 PM

another attempt to fix CI

zibi updated this revision to Diff 411431.Feb 25 2022, 9:03 AM
zibi marked 5 inline comments as done.

Trying to fix CI with static inline and addressing Daniel's comments.

zibi updated this revision to Diff 411503.Feb 25 2022, 12:51 PM
zibi added a comment.Feb 25 2022, 12:51 PM

rebase to fix CI

zibi updated this revision to Diff 411631.Feb 26 2022, 11:41 AM

NFC to pick up the fix for std_format_spec_string_unicode.pass.cp and make CI clean.

zibi updated this revision to Diff 411641.Feb 26 2022, 1:12 PM

Switched to new header <threading_support.h> introduced in https://reviews.llvm.org/D106124?id=411574.

zibi updated this revision to Diff 411650.Feb 26 2022, 7:31 PM

Move threading_support.h to libcxx/include to be together with __threading_support.

zibi updated this revision to Diff 411655.Feb 26 2022, 9:31 PM

formatting

zibi updated this revision to Diff 411657.Feb 26 2022, 9:57 PM

Attempt to fix Generated output in CI

DanielMcIntosh-IBM edited the summary of this revision. (Show Details)Mar 1 2022, 8:19 AM
zibi added a comment.Mar 7 2022, 10:14 AM

waiting for the review...

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 10:14 AM
zibi updated this revision to Diff 414255.Mar 9 2022, 5:56 PM

Rebase and undo changes for reverted commit 5aaefa51.

DanielMcIntosh-IBM edited the summary of this revision. (Show Details)Mar 10 2022, 11:54 AM
EricWF added a subscriber: EricWF.Mar 10 2022, 1:35 PM
EricWF added inline comments.
libcxx/src/include/internal_threading_support.h
28

I think we're very trying to avoid including <cassert> anywhere within libc++ headers.

31

Why static inline?

36

These declarations are different every time they appear..

63

This would need to be a reserved identifier.

77

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?

179

No assert. Use one of the abort methods.

libcxxabi/src/cxa_exception_storage.cpp
96

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?

libcxxabi/test/test_exception_storage.pass.cpp
19–20

This diff is hard to read because of all the whitespace changes.
What has actually changed here?

EricWF requested changes to this revision.Mar 10 2022, 1:44 PM

The entire point of the <__threading_support> interface is that it allows vendors/system maintainers to provide their own implementation without needing to change libc++.
I don't see why this patch couldn't use that same mechanism?

The libc++abi changes might need to live upstream, but the rest wouldn't.

This revision now requires changes to proceed.Mar 10 2022, 1:44 PM
zibi marked an inline comment as done.Mar 10 2022, 2:00 PM
zibi added inline comments.
libcxxabi/test/test_exception_storage.pass.cpp
19–20

I decided to do clang-format on the entire file since CI was failing and suggesting indentation which was not inline with the rest of the file. The file is relatively small. The relevant changes are inside the main():

#ifdef _LIBCXXABI_HAS_NO_THREADS
  size_t thread_globals;
  thread_code(&thread_globals);
  // Check that __cxa_get_globals() is not NULL.
  return (thread_globals == 0) ? 1 : 0;
#else  // !_LIBCXXABI_HAS_NO_THREADS
  // If threads are disabled at runtime, revert to single-threaded test.
  if (!__libcpp_is_threading_api_enabled()) {
    thread_code((void*)thread_globals);
    // Check that __cxa_get_globals() is not NULL.
    return (thread_globals[0] == 0) ? 1 : 0;
  }
libcxx/src/include/internal_threading_support.h
28

Even for internal headers? (Note that this file is under libcxx/src/include rather than libcxx/include)

31

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.

36

I'm not sure what you mean?

Since there's no standard way of checking whether the base threading library (e.g. pthreads) is available, each platform which needs to support runtime-dependent threading will have to write their own implementation of these functions (line 40-54). For example, AIX would change the #if on line 29 to #if defined(__MVS__) || defined(_AIX), and add it's own implementation before line 49 (guarded by an # elif defined(_AIX)).

Since each platform will need to provide their own implementation, we've provided a separate forward declaration to ensure the signature is always the same across all platforms (line 29-38). However, on platforms which don't have runtime-dependent threading (i.e. they know at the time we compile libcxx/libcxxabi whether a thread library like pthreads is available) the default, trivial implementation we provide should ideally declare these functions as constexpr. To do this we need to provide two separate versions of the forward declaration.

63

Probably a good idea to use a reserved identifier anyways, but this is internal to libcxx/libcxxabi, so it's impossible for this to produce any negative interaction with user code. Note that every symbol in this file is static inline.

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.

libcxx/src/include/internal_threading_support.h
28

Sorry, I see now that this isn't strictly a libc++ header.

44

s/should not/does not Because that's necessary for correctness.
When exactly at runtime does this property become fixed?

63

Disregard that comment. I see I was mistakne.

68

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?

73

Can __libcpp_might_have_multiple_threads() change during program execution? Or is its value fixed after program load?

85

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.

92

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; }
171

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,

libcxx/src/locale.cpp
730

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?

libcxx/src/locale.cpp
730

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

libcxxabi/src/cxa_exception_storage.cpp
96

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

zibi marked 19 inline comments as done.Mar 11 2022, 8:10 AM

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.

libcxx/src/include/internal_threading_support.h
31

The static inline was chosen so they are optimized out on the platforms which do not have dynamic threading support.

68

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.

73

It can change during the execution of the program.

77

We will have to experiment to see if that is even fisible.

92

The __libcpp_might_have_multiple_threads can change during the execution.

171

The value of __libcpp_is_threading_api_enabled() can only change at the load time.

179

Can you provide an example for it?

libcxx/src/locale.cpp
730

Will have a look at that. @DanielMcIntosh-IBM are then any plans to make changes to call_once() in future patches?

libcxxabi/src/cxa_exception_storage.cpp
96

I think so, this is done in single threaded case.

zibi updated this revision to Diff 414666.Mar 11 2022, 8:11 AM
zibi marked 5 inline comments as done.

Fix a typo.

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

libcxx/src/include/internal_threading_support.h
44

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)

68

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.

73

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.

77

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.

85

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.

171

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.

libcxx/src/locale.cpp
730

Will have a look at that. @DanielMcIntosh-IBM are then any plans to make changes to call_once() in future patches?

Not at the moment. The plan is to apply D117372 and D117373, then leave all of the C++11 Thread Support library unsupported/UB when threads are disabled.
This gives a very clear and sensible division between what is and isn't supported, rather than some vague, best-effort system, or listing every unsupported function one at a time.
As I discuss here, IMO this is what we should have done for _LIBCPP_HAS_NO_THREADS, but it might be too late for that now.

Update: it appears that [atomic] in the standard has been moved under [thread] and what was previously known as the "Thread Support Library" is now the "Concurrency support library" (see https://github.com/cplusplus/draft/commit/d74c2170a9f4c928519461d7742293af2d141852). This changes how we have to word things in documentation and comments, but that's about it. E.g. now the plan would be "When threads are disabled, support of the C++11 Concurrency Support Library will be limited to atomic operations."

zibi updated this revision to Diff 415902.Mar 16 2022, 10:42 AM
zibi marked 3 inline comments as done.

This revision updates __libcpp_execute_once() according to the comments.

ldionne added inline comments.Mar 16 2022, 11:43 AM
libcxx/src/include/internal_threading_support.h
77

Does this mean that users can't override operator new/operator delete by providing a strong definition in their program, i.e. that it's impossible to have a conforming C++ implementation on z/OS?

libcxx/src/include/internal_threading_support.h
77

At the moment, yes. Any invocation of new and delete within the standard library DLL will not use any user-defined operator new\operator delete. This is something that would need to be fixed in the OS, and I'm not aware of any timeline or plans for fixing it right now.

Win32 DLL has the same problem, and had to disable portions of tests for that reason. D107755 made their solution more generic so it could be used for z/OS.

zibi updated this revision to Diff 416378.Mar 17 2022, 8:03 PM
zibi marked 8 inline comments as done.

This revision reduced wrapper functions in internal_threading_support.h to just
__libcpp_mutex_lock_wrapper() and __libcpp_mutex_unlock_wrapper() with the new suffix.
This is a bare minimum needed for POSIX(OFF) on z/OS.

It also added the guard for z/OS was added in locale::id::__get() for POSIX(OFF) path.

Lastly, the formatting in test_exception_storage.pass.cpp was removed to help the review.

DanielMcIntosh-IBM added inline comments.
libcxxabi/test/test_exception_storage.pass.cpp
53–55

This indentation should match the rest of the if

zibi added inline comments.Mar 18 2022, 9:04 AM
libcxxabi/test/test_exception_storage.pass.cpp
53–55

Yes, I will wait if there are any other requests and do them together otherwise, I'm planning to clang-format the entire file as I had before in a separate subsequent commit.

zibi added a comment.Mar 28 2022, 1:44 PM

@ldionne Louis, this is one of the reviews I DM you about.