Page MenuHomePhabricator

[SystemZ][z/OS] Add ASCII and 32-bit variants for libc++.
Needs ReviewPublic

Authored by zibi on Jan 28 2022, 11:43 AM.

Details

Reviewers
muiez
SeanP
DanielMcIntosh-IBM
fanbo-meng
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Restricted Project
Summary

This patch enables libc++ build as shared library in all combinations of ASCII/EBCDIC and 32-bit/64-bit variants. In particular it introduces:

  1. ASCII version of libc++ named as libc++_a.so
  2. Script to rename DLL name inside the generated side deck
  3. Various names for dataset members where DLL libraries and their side decks will reside

Background and rational of this patch

The linker on z/OS creates a list of exported symbols in a file called side deck. The list contains the symbol name as well as the name of the DLL which implements the symbol. The name of the DLL depends on what is specified in the -o command line option. If it points to a USS file, than the DLL name in the side deck will be the USS file name. If it points to a member of a dataset then the DLL name in the side deck is the member name.

If CMake could deal with z/OS datasets we could use -o that points to a dataset member name, but this does not seem to work so we have to produce a USS file as the DLL and then copy the content of the produced side deck to a dataset as well as rename the USS file name in the side deck to a dataset member name that corresponds to that DLL.

Diff Detail

Unit TestsFailed

TimeTest
87,230 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class::try_lock_shared.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8006/llvm-project/libcxx-ci/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_shared.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -I /scratch/powerllvm/cpap8006/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8006/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -nostdlib++ -L /scratch/powerllvm/cpap8006/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8006/llvm-project/libcxx-ci/build/aix/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/Output/try_lock_shared.pass.cpp.dir/t.tmp.exe

Event Timeline

zibi created this revision.Jan 28 2022, 11:43 AM
zibi requested review of this revision.Jan 28 2022, 11:43 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 28 2022, 11:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
muiez accepted this revision.Feb 4 2022, 10:12 AM

LGTM

zibi added a comment.Feb 11 2022, 5:46 AM

@libc++ ping

zibi updated this revision to Diff 408896.Feb 15 2022, 8:49 AM

Rebase with latest and removing conflicts.

SeanP accepted this revision.Feb 15 2022, 10:39 AM

We'll have to create another patch to apply the side deck renaming to the libc++exception added in https://reviews.llvm.org/D118620/new/.

Otherwise LGTM

zibi updated this revision to Diff 409265.Feb 16 2022, 7:58 AM

NFC: just trying to clean CI status

zibi added a comment.Feb 22 2022, 12:37 PM
This comment was removed by zibi.
muiez added inline comments.Mar 2 2022, 7:24 AM
libcxx/src/CMakeLists.txt
230–233

I don't think we need this line since the COMPILE_FLAGS is already set for that target (line 218)

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 7:24 AM
muiez added inline comments.Mar 2 2022, 7:27 AM
libcxx/src/CMakeLists.txt
237

We don't need to set ZOS_BUILD_32_BITS here anymore, as it is not used.

muiez added inline comments.Mar 2 2022, 8:17 AM
libcxx/src/CMakeLists.txt
235

The LIBCXX_TARGET_TRIPLE is deprecated.

ldionne requested changes to this revision.Mar 2 2022, 9:10 AM
ldionne added a subscriber: phosek.

I can't support this patch as-is. It upstreams vendor-specific quirks for a single platform, causing the generic upstream code to be side-by-side with platform-specific code for z/OS only. Please try to put yourself in the shoes of the open-source community that doesn't work on z/OS on a day to day basis: this patch basically adds a bunch of platform-specific complexity (i.e. tech debt) that everyone will have to deal with, for essentially only z/OS's benefit. This is not acceptable.

Take for instance the example of Fuchsia, another platform that uses libc++. @phosek added support for a bunch of stuff to support Fuchsia's build, for example LIBCXX_HERMETIC_STATIC_LIBRARY & friends, or LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY. Even though I'm not a fan of those because they increase complexity, they are generic and one could argue they are generally useful, so it makes sense to try and support them upstream. I am not sure whether Fuchsia is using the CMake build at this point (I don't think it does yet), however my point is this: how many references to Fuchsia specific stuff can you count in our CMakeLists.txt? Very few, because they found ways to implement most of what they needed generically.

Similarly, at Apple, we used to build libc++ and libc++abi with a Xcode project a couple of years ago. I did a bunch of work to migrate that build over to CMake, and in doing so, I had to introduce a bunch of Apple-specific tweaks to the CMake build. I have been upstreaming things that make sense one tiny bit at a time, but even three years later, we are still living with a downstream diff that I don't think would make sense for upstream to support, so I'm keeping it downstream until I can find reasonable ways to upstream it all. It would have been much easier to just upstream the whole thing from the start, however LLVM is not a dumping ground for vendors, so it would have been bad open-source citizenship to do that.

So:

  • Libc++ welcomes contributions for adding support to new platforms -- it makes it more widely useful, which is awesome.
  • Libc++ is not a dumping ground for arbitrary vendor-specific stuff -- there's too many vendors with too many use cases that require tweaking libc++, we couldn't realistically accommodate everyone even if we wanted to.
  • Having some amount of downstream-only diff is normal. The key is to manage it to reduce downstream merge conflicts.

Also, the devil's in the details -- I am much less concerned by adding something self-contained like zos_rename_dll_side_deck.sh than tweaking the CMake itself, since we can basically forget about zos_rename_dll_side_deck.sh without harm, however we'll have to deal with the CMake complexity forever. Of course, upstreaming zos_rename_dll_side_deck.sh on its own doesn't make much sense -- I'm just trying to nuance my position.

FWIW, I think there might be ways to achieve some of what you want in a way that benefits everyone. Several vendors need to run post-install scripts or have slightly more complicated ways of building the library (see for example libcxx/utils/ci/apple-install-libcxx.sh, which I consider terrible tech debt for upstream and want to burn with fire). I suspect it may be possible to add customization points to the CMake build in a way that vendors can more easily configure it without adding tech debt for everyone, kind of like what we did for new style testing configurations (see for example libcxx/test/configs/apple-libc++-backdeployment.cfg.in).

libcxx/include/__config_site.in
13

We don't put any logic inside __config_site. This logic should instead be implemented by setting a different namespace in the cache you use when building for EBCDIC/ASCII (you probably want two caches).

libcxx/utils/libcxx/test/config.py
377 ↗(On Diff #409265)

You should be using a new style config (see libcxx/test/configs/), not the legacy system. This is much easier to tweak for your exact needs.

This revision now requires changes to proceed.Mar 2 2022, 9:10 AM
muiez added inline comments.Mar 2 2022, 10:05 AM
libcxx/utils/libcxx/test/config.py
377–378 ↗(On Diff #409265)

We can avoid this, since we moved to the new config file. Will create a separate patch for this.

SeanP added inline comments.Mar 7 2022, 1:29 PM
libcxx/include/__config_site.in
13

Unless we are missing something, I don't think this kind of requirement is reasonable. We need to be able to the distinguish between ebcdic/ascii with the installed headers. If we don't put conditional logic into the __config header, we will be creating horribly complicated code (if it's even possible) in the compiler. If we can't put conditional logic into __config, we will be required to ship two copies of the the libc++ headers (one where __config is set up for ascii and one where it is set up for ebcdic). This is going to force the compiler to change the include dir for libc++ based on the the ascii/ebcdic option. That is going to break the -isystem option.

We need one set of header files that defines _LIBCPP_ABI_NAMESPACE differently at compile time depending on if it is ascii or ebcdic. This is the only way we can see doing this. If there is a better way we are open to doing it that way. The cache files don't work as they would result in two different set of headers.

SeanP added inline comments.Mar 7 2022, 1:47 PM
libcxx/src/CMakeLists.txt
456

I'd either wrap all of these install commands in one if (not ZOS) or get rid of them completely.

SeanP added inline comments.Mar 7 2022, 2:46 PM
libcxx/src/CMakeLists.txt
456

Just to be clear, I meant get rid of the "NOT ZOS AND" not the install commands.

zibi updated this revision to Diff 413931.Mar 8 2022, 2:17 PM
zibi marked 7 inline comments as done.

This patch addresses the comments from Sean and Muiez. It reduces the original patch and makes it up-to-date.

zibi updated this revision to Diff 414441.Mar 10 2022, 11:19 AM

The side deck was created in ASCII mode which is not desired. This revision corrects that.