Page MenuHomePhabricator

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

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

Details

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
  4. Add the following options:
    • LIBCXX_SHARED_OUTPUT_NAME
    • LIBCXX_ADDITIONAL_COMPILE_FLAGS
    • LIBCXX_ADDITIONAL_LIBRARIES
    • LIBCXXABI_ADDITIONAL_COMPILE_FLAGS
    • LIBCXXABI_ADDITIONAL_LIBRARIES

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

zibi updated this revision to Diff 444061.Jul 12 2022, 1:13 PM
  • Rebasing and resolving conflicts

No functional changes are introduced since the last patch.

zibi added a comment.Jul 12 2022, 1:34 PM

The CI is clean now, Can I have the approval or more request, Thx.

zibi updated this revision to Diff 444335.Jul 13 2022, 10:47 AM

Rebase to pick up 0d7859765260e1dddfdc3490c1fe35537775bb41 which hopefully will fix CL.

zibi added a comment.Jul 22 2022, 7:21 AM

@ldionne Louis, all comments have been addressed CI is green, can you look at it again? Thx.

zibi added a comment.Aug 3 2022, 12:09 PM

ping, still waiting for the review ...

ldionne requested changes to this revision.Aug 3 2022, 1:06 PM

I left some comments. This still doesn't meet the bar since it hardcodes a bunch of z/OS specific knowledge in the build. However, I can see ways out of it, as explained in the comments.

The CI is clean now, Can I have the approval or more request, Thx.

AFAICT we don't have z/OS CI set up anyway, so it wouldn't show up even if the patch didn't work. At least we do know it does not break other configurations, indeed.

libcxx/include/__config_site.in
13 ↗(On Diff #409265)

If we don't put conditional logic into the __config header

Logic inside __config is fine, but logic inside __config_site is not. This is adding logic inside __config_site.

We made changes to Clang to make it easy to ship libc++ headers as two things:

  1. A per-target include directory that contains __config_site (and only that)
  2. A target-agnostic c++/v1 directory that contains all the other headers

Then, the driver selects the right target-specific directory so the right __config_site header is picked up. I believe you should be using that instead. If you did, it would actually be quite nice: there wouldn't be any specific logic here since you'd define a different _LIBCPP_ABI_NAMESPACE in each build of libc++ that you do (ASCII and non-ASCII), and that would result in two different __config_sites in different directories.

libcxx/src/CMakeLists.txt
289

The only things that this target does differently from cxx_shared is pass -fzos-le-char-mode=ascii, set the output name to c++_a and add a post-build command.

Instead, the following would avoid hardcoding platform-specific knowledge in our build:

  1. Build twice, with CMAKE_CXX_COMPILE_FLAGS=-fzos-le-char-mode=ascii|ebcdic. This would also remove the need for setting add_target_flags_if_supported("-fzos-le-char-mode=ebcdic") in libcxxabi/CMakeLists.txt
  2. Add a way to customize the output name of libc++ (by default it would be c++, but you'd set it to c++_a in your CMake cache)
  3. Add your post-build step to cxx_shared under some option. This isn't clean, but I could refactor that eventually to provide a way to run arbitrary post-build steps more cleanly.
This revision now requires changes to proceed.Aug 3 2022, 1:06 PM
zibi requested review of this revision.Aug 17 2022, 9:56 AM

Louis, thank you for the review and providing some suggestions. However, I'm still not sure how to proceed see my inline comments.

libcxx/CMakeLists.txt
466–467

The add_target_flags_if_supported("-fzos-le-char-mode=ebcdic") will be removed since I found an alternative way.

libcxx/include/__config_site.in
13 ↗(On Diff #409265)

I'm not entire sure how you envision this. You can not move the logic to __config since #cmakedefine has to be preprocessed by cmake. More importantly I think you asking for 2 separate builds which we want to avoid. See my other inline comment.

libcxx/src/CMakeLists.txt
289

Does current build infrastructure allow to reuse the previous build and build only c++?

We build c++ only in both encoding modes and the rest of the libraries in EBCDIC. The c++ has dependency on rest of the libraries like c++_abi, unwinder, etc. We did not want to duplicated the entire library in both modes.

The first build would be the EBCDIC one and that is all good. However, the second ASCII build would throw away anything but c++_a. The most importantly issue would be how to make dependency to EBCDIC libraries from the 1st build rather then ASCII build from the 2nd one.

libcxxabi/CMakeLists.txt
244–247

The add_target_flags_if_supported("-fzos-le-char-mode=ebcdic") will be removed since I found an alternative way.

zibi updated this revision to Diff 458549.Sep 7 2022, 1:37 PM

Addressing Louis's comments to hide z/OS changes as much as possible in cache.

ldionne requested changes to this revision.Sep 8 2022, 12:06 PM
ldionne added inline comments.
libcxx/CMakeLists.txt
467–469

Instead, let's add an option like LIBCXX_ADDITIONAL_COMPILE_FLAGS that we can use to pass those compiler flags from the cache. I'm not a huge fan of doing that, but I do think it's a useful backdoor to have.

However, even with LIBCXX_ADDITIONAL_COMPILE_FLAGS, I don't understand why -I../${CMAKE_CXX_COMPILER_TARGET}/${LIBCXX_INSTALL_INCLUDE_DIR} is needed. What purpose does it serve?

libcxx/cmake/caches/s390x-ibm-zos-ascii.cmake
16–17

Why are we setting LLVM_FOO variables in the CMake cache for libc++? Unless I am mistaken, I think neither of those two cache values will be picked up by anything in the runtimes build, and so they serve no purpose AFAICT.

libcxx/cmake/caches/s390x-ibm-zos.cmake
26

Same here, this does not exist.

libcxx/cmake/caches/s390x32-ibm-zos-ascii.cmake
23

I don't see this used anywhere. Should this be defined in CMakeLists.txt and then overriden here only with set(LIBCXX_SHARED_OUTPUT_NAME "c++_a")?

25

This is not used anywhere. Is there a reason why it's in all the caches?

libcxx/src/CMakeLists.txt
203–204

Why do you avoid linking cxx_shared against its dependencies in the z/OS build? Is it because you disable building the headers? I don't think this should be z/OS specific, this should probably be supported out of the box instead.

This revision now requires changes to proceed.Sep 8 2022, 12:06 PM
zibi marked an inline comment as done.Sep 8 2022, 12:57 PM

answering comments and some questions

libcxx/CMakeLists.txt
467–469

The -I.. is needed because we build only cxx_shared as ASCII and need to point to headers cxx-headers and cxxabi-headers from the full build we do in EBCDIC.

libcxx/cmake/caches/s390x-ibm-zos-ascii.cmake
16–17

Yes, you are right I will remove them.

libcxx/cmake/caches/s390x32-ibm-zos-ascii.cmake
23

Yes, this is how I have implemented it in my local branch. For some reason it was not included in this patch. I will correct this.

25

This is needed for the exception library which we created by splitting cxx which is not included in this patch but it will be included in separate patch. The reason I'm keeping it here so I can easy verify current patch on my local branch.

libcxx/src/CMakeLists.txt
203–204

Yes we disable everything including headers when building cxx_shared in ASCII and want to link with dependencies from the full separate build done in EBCDIC. This is why we need -I and -Wl,<side-deck>.x options.

What do you mean out of the box. AFAIK, nobody builds only libcxx and depend on previous builds on its dependencies. Maybe I should have another variant of LIBCXX_CXX_ABI.

zibi updated this revision to Diff 459594.Sep 12 2022, 5:07 PM
  • Make cxx_shared library common for EBCDIC and ASCII build variations.
  • Introducing built-libcxxabi to pick up abi headers from previous build.
  • Adding the following macros:

    LIBCXX_DLL_NAME LIBCXX_SHARED_OUTPUT_NAME LIBCXX_ADDITIONAL_COMPILE_FLAGS LIBCXX_ADDITIONAL_LIBRARIES
muiez added inline comments.Sep 14 2022, 11:16 AM
libcxxabi/CMakeLists.txt
231

Typo

zibi marked 5 inline comments as done.Sep 15 2022, 7:04 AM

Putting rational why I opted out for LIBCXX_CXX_ABI=built-libcxxabi.

libcxx/cmake/Modules/HandleLibCXXABI.cmake
137 ↗(On Diff #459594)

Why do I need another LIBCXX_CXX_ABI?

The system-libcxxabi does not work for us since imported_library cannot find libc++abi.so. This is because find_library works only with .x extension whereas CMAKE_SHARED_LIBRARY_SUFFIX is set to .so. We could use CMAKE_IMPORT_LIBRARY_SUFFIX instead which on z/OS is .x however, cmake does not set CMAKE_IMPORT_LIBRARY_SUFFIX on other platforms (Linux on Power for example).

Forcing .x extension and modifying LIBCXX_CXX_ABI_LIBRARY_PATH to point to previous build, which we want to reuse for ASCII build, the side deck libc++abi.x can be found however, the link step fails with:

ninja: error: 'libcxx-abi-shared-NOTFOUND', needed by 'lib/libc++_a.so', missing and no known rule to make it

Further by removing target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared) will lead to

The archive library liblibcxx-abi-shared.a cannot be found.
This is because we have -llibcxx-abi-shared on the link step whereas we supposed to have <absolude path>/libc++abi.x in LINK_LIBRARIES.

zibi updated this revision to Diff 461946.EditedSep 21 2022, 10:39 AM
  • Make cxx_shared library common for EBCDIC and ASCII build variations.
  • Use LIBCXX_CXX_ABI=system-libcxxabi to pick up abi headers and library from previous build.
zibi updated this revision to Diff 461975.EditedSep 21 2022, 11:52 AM
zibi marked an inline comment as done.
  • Add the following options to documentation:
  • LIBCXX_SHARED_OUTPUT_NAME
  • LIBCXX_ADDITIONAL_COMPILE_FLAGS
  • LIBCXX_ADDITIONAL_LIBRARIES
  • LIBCXXABI_ADDITIONAL_LIBRARIES
zibi edited the summary of this revision. (Show Details)Sep 21 2022, 12:10 PM
zibi updated this revision to Diff 461981.Sep 21 2022, 12:29 PM

Looks like my last commit was skipped. I'm adding it now.

zibi updated this revision to Diff 462556.Sep 23 2022, 11:11 AM
Fix just one typo. @dionne I think I addressed all your comments. Please review it.
zibi added a comment.Sep 23 2022, 11:13 AM

@ldionne I think I addressed all your comments.
Please review it.

ldionne requested changes to this revision.Sep 29 2022, 11:46 AM

Only a few nits.

libcxx/CMakeLists.txt
472–473

Let's move this to the per-target functions below. Let's put it in cxx_add_basic_build_flags. It should then become target_compile_options(...) and target_link_libraries(...).

libcxx/cmake/caches/s390x32-ibm-zos.cmake
17

This variable doesn't exist, why is it in the cache? If you've got internal-only changes you're keeping downstream, it looks like this should be part of that.

19

Same here regarding internal-only changes.

libcxxabi/CMakeLists.txt
230–234

We should also have LIBCXXABI_ADDITIONAL_COMPILE_FLAGS.

This revision now requires changes to proceed.Sep 29 2022, 11:46 AM
zibi updated this revision to Diff 464087.Sep 29 2022, 4:23 PM
zibi marked 4 inline comments as done.
Add LIBCXXABI_ADDITIONAL_COMPILE_FLAGS and other changes requested by Louis.
zibi added a comment.Sep 29 2022, 4:26 PM

Ready for another round. FYI @ldionne

ldionne accepted this revision.Sep 30 2022, 6:01 AM
This revision is now accepted and ready to land.Sep 30 2022, 6:01 AM
zibi updated this revision to Diff 464726.Oct 3 2022, 10:17 AM

Fix CI build.

zibi updated this revision to Diff 464764.Oct 3 2022, 12:16 PM

Fix CI - Documentation

zibi edited the summary of this revision. (Show Details)Oct 3 2022, 12:19 PM
This revision was automatically updated to reflect the committed changes.