[SystemZ][z/OS] Move several exception derived classes to c++abi library

[fanbo-meng]

Summary

The following classes derived from std::exception have member function what() which will be moved to c++abi along with some destructors.

• std::__1::bad_weak_ptr
• std::bad_variant_access
• std::bad_any_cast
• std::bad_optional_access

In addition, we will drop the version namespace for bad_weak_ptr and have std::bad_weak_ptr to be consistent with other classes.

This change is the base for the following patch addressing the encoding for strings representing the exception types.

[muiez]

LGTM

[fanbo-meng]

ping :)

[fanbo-meng]

Update zos cmake cache

[SeanP]

LGTM

[fanbo-meng]

rebase to latest main

[fanbo-meng]

run clang-format

[fanbo-meng]

formatting __config; rename new sources files to have unique names

[zibi]

LGTM

libcxx/cmake/caches/zos.cmake

9 ↗

(On Diff #409631)

This suggestion reflects the recent removal of standalone build.

[ldionne]

libcxx/src/exceptions/exception_any.cpp

9 ↗

(On Diff #409631)

Please use <any> instead of "any". Also applies below.

11 ↗

(On Diff #409631)

I think this should be using _LIBCPP_BEGIN_NAMESPACE_STD_EXCEPTION. Same for others below.

[ldionne]

libcxx/include/__config

801–804 ↗

(On Diff #409631)

It seems to me that we *always* want the namespace for exceptions to be namespace std only, without the ABI namespace.

I think it only falls down for bad_weak_ptr, which we seem to have defined in std::__1 instead of std::. Is that correct?

libcxx/src/CMakeLists.txt

289

I think there's something greater trying to emerge here. We already define some exception types inside libc++abi in namespace std. We should have a consistent approach with all those. In fact, perhaps what we want is to create either a static archive or a shared library with all those exception types, from libc++ and libc++abi. The default configuration would be to statically link that library into libc++, but other configurations could be possible, like leaving it be its own dylib (your use case).

libcxx/src/exceptions/exception_variant.cpp

13 ↗

(On Diff #409631)

Please don't change the formatting needlessly. Applies everywhere.

libcxx/utils/libcxx/test/config.py

377 ↗

(On Diff #409631)

Please define a configuration file in the likes of libcxx/test/configs/apple-libc++-shared.cfg.in for testing. We're trying to move away from this config.py.

[fanbo-meng]

libcxx/include/__config

801–804 ↗

(On Diff #409631)

That's correct, in our case we want them to have std:: namespace only so we define _LIBCPP_BEGIN_NAMESPACE_STD_EXCEPTION for bad_weak_ptr .
For the other three classes they are already using only std:: namespace, that's why we kept their existing std:: namespace in the new files to avoid inconsistency.

libcxx/src/CMakeLists.txt

289

Our initial approach was to move all exception symbols from libc++ to libc++abi, but then we discovered some exception classes have dependencies on libcxx symbols, for example runtime_error(const string& msg) requires access to string methods. Therefore they have to be left in libc++ so I don't think it would be possible to move everything into a unified library.

[fanbo-meng]

libcxx/src/exceptions/exception_any.cpp

11 ↗

(On Diff #409631)

We used std namespace here to preserve the original behaviour of this class. The exception derived classes aren't really consistent with namespace as some uses std:: and others uses std::__1::, so our use of _LIBCPP_BEGIN_NAMESPACE_STD_EXCEPTION is intended for those that originally uses std::__1:: namespace, like bad_weak_ptr class.

libcxx/utils/libcxx/test/config.py

377 ↗

(On Diff #409631)

https://reviews.llvm.org/D118503#3354937 @muiez will have another patch for this.

[fanbo-meng]

Addressing comments

[zibi]

LGTM

[zibi]

libcxx/src/CMakeLists.txt

305

I just noticed that we are missing the change for renaming dll name inside the side deck file. However this can be done in a separate patch.

[fanbo-meng]

ping :) @libcxx

[philnik]

libcxx/include/__memory/shared_ptr.h

50

Is it possible that this was just an oversight? If that's the case, maybe this should just be an ABI flag instead of getting it's own namespace macros?

[zibi]

• Rebase and move recipe to build c++_exception to separate cmake.

[zibi]

• config.py should not be part of this change

[zibi]

• formating changes and make set_target_properties for cxx_exception be consistent with cxx_shared

[zibi]

more formating

[zibi]

• switch to use override in place of virtual

[fanbo-meng]

libcxx/include/__memory/shared_ptr.h

50

This is intended, as we want std namespace for bad_weak_ptr for z/OS.

[zibi]

• use system notation for #include directive

[zibi]

@ldionne @philnik Please have a look again and let me know if you want to change anything.

[philnik]

LGTM with comments applied. Leaving final approval to @ldionne.

libcxx/src/exceptions/any.cpp

1 ↗

(On Diff #465716)

I wouldn't bother with putting every one of these into it's own file.

libcxx/src/exceptions/memory.cpp

2–9 ↗

(On Diff #465716)

The license headers seem to be broken in a few places.

[zibi]

• consolidate multiple exceptions files into a single file

[zibi]

Thank you for all reviews which are not addressed.
@ldionne Can you have a look? I just need your stamp of approval.

[zibi]

ping @ldionne

[zibi]

Louis, this has been reviewed by others including yourself and is waiting for your stamp of approval, @ldionne

[zibi]

ping @ldionne

[zibi]

ping

[EricWF]

I don't really like the state this leaves variant, any and friends in when exception vtables aren't compiled into the dylib.
There's no way to know only from the headers that using any of these types will result in linker errors.

Yes, is the Z/OS case you provide these symbols via a different means, but that's not clear anywhere in the CMake files as written.

I think the source changes look acceptable, but the changes to the build system may not be changes we want in upstream. There has been a enormous amount of work to simply and remove all sorts of cryptic build-time switches in order to make our CMake files understandable, and I see this change as going backwards in that respect.

libcxx/include/__memory/shared_ptr.h

50

I don't think we want to formalize this different exception namespace. Especially as a versioned one (which it is by default).

The correct thing to do is place these types directly in namespace std, so their mangling never changes. bad_weak_ptr should have never gone in the versioned namespace, but that mistake was made, and now we're stuck with it (for now).

I'm OK with this change, but lets be verbose and do the ugly #ifdef blocks and namespace switching directly around bad_weak_ptr.

libcxx/src/CMakeLists.txt

139

This should be gated on the other CMake switch you're introducing, no?

[zibi]

• Remove _LIBCPP_BEGIN_NAMESPACE_EXCEPTION and _LIBCPP_END_NAMESPACE_EXCEPTION

[zibi]

Addressing comments...

libcxx/include/__memory/shared_ptr.h

50

sure

libcxx/src/CMakeLists.txt

139

Not really, we build the library multiple times to support ASCII and EBCDIC but the exception is being build only once in EBCDIC encoding. Using other CMake switch will build cxx with and without exception. On z/OS we don't want to build exception as part of cxx. This is the fundamental reason why we doing this change.

[zibi]

• correct typo

[EricWF]

Sorry, I'm going to have to let @ldionne weigh in on this too. I'm just not sure introducing this whole new build mode with a separate exception library is something we want to do -- so I would like input from the other maintainers.

I think the correct approach here is to define the required exception symbols in libc++abi, like we do already for most exception types.

I think we're inventing something novel here because it's easy, not because it's the best approach.

libcxx/include/__memory/shared_ptr.h

50

Let's create an ABI macro, since other users might want to have the bad_weak_ptr exception too, that will make this code more readable and then you can just turn it on for your platform.

Sorry for not suggesting that earlier.

libcxx/src/CMakeLists.txt

139

Right, then these things should go into libc++abi. Not a third separate library.

[zibi]

@EricWF Thank you for the review.

@ldionne Can you weigh on this as requested by Eric?

libcxx/include/__memory/shared_ptr.h

50

@ldionne Do you agree with Eric's suggestion?
Also I believe we HAD a macro before but it was suggested to remove it and just use namespace std directly. If the recommendation is to go back to macro idea I wonder where should it go since it is needed here as well in libcxx/src/exceptions.cpp.

libcxx/src/CMakeLists.txt

139

See the discussion below between Fanbo and Loius why we created 3rd library. We really cannot undo this change.

[ldionne]

Recording the conclusion of our live meeting w/ Eric today: We're asking that these be moved into libcxxabi like the other exception types, which should not require any z/OS specific configuration option.

[zibi]

Moving several exception derived classes c++abi to address review comments.

[zibi]

• trying to fix CI.

[zibi]

• fix formatting

[zibi]

@fanbo-meng Can you close this review?
I'm going to continue in D138951.

libcxx/cmake/Modules/HandleExceptionLibrary.cmake

-#===============================================================================
-# Define targets for linking against the selected ABI library
-#
-# After including this file, the following targets are defined:
-# - cxx_exception: A target representing the shared c++_exception library.
-#                  constituting the split from c++ shared library.
-#                  The LIBCXX_EXCEPTION_ZOS_BUILD, which can be defined in
-#                  cache, determines if the library will be built or not.
-#===============================================================================
-
-
-if (LIBCXX_EXCEPTION_ZOS_BUILD)
-  add_library(cxx_exception SHARED ${exclude_from_all} ${LIBCXX_EXCEPTIONS_SOURCE} ${LIBCXX_HEADERS})
-  target_link_libraries(cxx_exception PUBLIC cxx-headers
-                                      PRIVATE ${LIBCXX_LIBRARIES})
-  set_target_properties(cxx_exception
-    PROPERTIES
-      COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
-      LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
-      OUTPUT_NAME   "c++_exception"
-      VERSION       "${LIBCXX_ABI_VERSION}"
-      SOVERSION     "${LIBCXX_ABI_VERSION}"
-      DEFINE_SYMBOL ""
-      POSITION_INDEPENDENT_CODE ON
-  )
-  cxx_add_common_build_flags(cxx_exception)
-  cxx_set_common_defines(cxx_exception)
-
-  add_custom_command(TARGET cxx_exception POST_BUILD
-    COMMAND
-      ${LIBCXX_SOURCE_DIR}/utils/zos_rename_dll_side_deck.sh
-      $<TARGET_LINKER_FILE_NAME:cxx_exception> $<TARGET_FILE_NAME:cxx_exception> "${LIBCXX_EXCEPTION_DLL_NAME}"
-    COMMENT "Rename dll name inside the side deck file"
-    WORKING_DIRECTORY $<TARGET_FILE_DIR:cxx_exception>
-  )
-
-  target_link_libraries(cxx_exception PUBLIC libcxx-abi-shared)
-  list(APPEND LIBCXX_BUILD_TARGETS "cxx_exception")
-endif()

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

 set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXXABI_INCLUDE_TESTS OFF CACHE BOOL "")
 
 # Target Specific
 set(LIBCXX_DLL_NAME CRTEQCXS CACHE STRING "")
 set(LIBCXX_SHARED_OUTPUT_NAME "c++_a" CACHE STRING
     "Output name for the shared libc++ runtime library.")
-set(LIBCXX_EXCEPTION_ZOS_BUILD OFF CACHE BOOL
-    "Build libcxx exception library on z/OS as standalone.")
 set(LIBCXX_CXX_ABI system-libcxxabi CACHE STRING "")
 
 set(LIBCXX_ADDITIONAL_COMPILE_FLAGS "-fzos-le-char-mode=ascii" CACHE STRING "")
 set(LIBCXX_ADDITIONAL_LIBRARIES "-L../s390x-ibm-zos/lib -Wl,../s390x-ibm-zos/lib/libunwind.x" CACHE STRING "")

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

 set(CMAKE_BUILD_TYPE Release CACHE STRING "")
 
 # Common
 set(LIBCXX_ENABLE_ABI_LINKER_SCRIPT OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXX_INCLUDE_TESTS ON CACHE BOOL "")
 
 set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXXABI_INCLUDE_TESTS ON CACHE BOOL "")
 
 # Target Specific
 set(LIBCXX_DLL_NAME CRTEQCXE CACHE STRING "")
-set(LIBCXX_EXCEPTION_DLL_NAME CRTEQCXP CACHE STRING "")
-set(LIBCXX_EXCEPTION_ZOS_BUILD ON CACHE BOOL
-      "Build libcxx exception library on z/OS as standalone.")
 
 set(LIBCXXABI_DLL_NAME CRTEQCXA CACHE STRING "")
 set(LIBCXXABI_ADDITIONAL_LIBRARIES "-Wl,lib/libunwind.x" CACHE STRING "")

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

 set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXXABI_INCLUDE_TESTS OFF CACHE BOOL "")
 
 # Target Specific
 set(LIBCXX_DLL_NAME CRTEHCXS CACHE STRING "")
 set(LIBCXX_SHARED_OUTPUT_NAME "c++_a" CACHE STRING
     "Output name for the shared libc++ runtime library.")
-set(LIBCXX_EXCEPTION_ZOS_BUILD OFF CACHE BOOL
-    "Build libcxx exception library on z/OS as standalone.")
 set(LIBCXX_CXX_ABI system-libcxxabi CACHE STRING "")
 
 set(LIBCXX_ADDITIONAL_COMPILE_FLAGS "-fzos-le-char-mode=ascii" CACHE STRING "")
 set(LIBCXX_ADDITIONAL_LIBRARIES "-L../s390x32-ibm-zos/lib -Wl,../s390x32-ibm-zos/lib/libunwind.x" CACHE STRING "")

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

 # Common
 set(CMAKE_BUILD_TYPE Release CACHE STRING "")
 
 set(LIBCXX_ENABLE_ABI_LINKER_SCRIPT OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXX_INCLUDE_TESTS ON CACHE BOOL "")
 
 set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXXABI_INCLUDE_TESTS ON CACHE BOOL "")
 
 # Target Specific
 set(LIBCXX_DLL_NAME CRTEHCXE CACHE STRING "")
-set(LIBCXX_EXCEPTION_DLL_NAME CRTEHCXP CACHE STRING "")
-set(LIBCXX_EXCEPTION_ZOS_BUILD ON CACHE BOOL
-    "Build libcxx exception library on z/OS as standalone.")
 
 set(LIBCXXABI_DLL_NAME CRTEHCXA CACHE STRING "")
 
 set(LIBCXXABI_ADDITIONAL_LIBRARIES "-Wl,lib/libunwind.x" CACHE STRING "")

libcxx/include/__memory/shared_ptr.h

 #if !defined(_LIBCPP_HAS_NO_ATOMIC_HEADER)
 #  include <atomic>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
-#if defined(__MVS__)
 namespace std {
-#else
-_LIBCPP_BEGIN_NAMESPACE_STD
-#endif
 class _LIBCPP_EXCEPTION_ABI bad_weak_ptr : public std::exception {
 public:
   bad_weak_ptr() _NOEXCEPT                    = default;
   bad_weak_ptr(const bad_weak_ptr&) _NOEXCEPT = default;
   ~bad_weak_ptr() _NOEXCEPT override;
   const char* what() const _NOEXCEPT override;
 };
 
-#if defined(__MVS__)
 }
-#else
-_LIBCPP_END_NAMESPACE_STD
-#endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // NOTE: Relaxed and acq/rel atomics (for increment and decrement respectively)
 // should be sufficient for thread safety.
 // See https://llvm.org/PR22803
 #if defined(__clang__) && __has_builtin(__atomic_add_fetch)          \
                        && defined(__ATOMIC_RELAXED)                  \

libcxx/src/CMakeLists.txt

if (LIBCXX_ENABLE_FILESYSTEM)
   # definitions. See filesystem/int128_builtins.cpp.
   if (NOT LIBCXX_USE_COMPILER_RT)
     list(APPEND LIBCXX_SOURCES
       filesystem/int128_builtins.cpp
       )
   endif()
 endif()
 
-set(LIBCXX_EXCEPTIONS_SOURCE
-  exceptions.cpp
-  )
-
-if (NOT ZOS)
-  list(APPEND LIBCXX_SOURCES ${LIBCXX_EXCEPTIONS_SOURCE})
-endif()
-
 # Add all the headers to the project for IDEs.
 if (LIBCXX_CONFIGURE_IDE)
   file(GLOB_RECURSE LIBCXX_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/../include/*)
   if(WIN32)
     file( GLOB LIBCXX_WIN32_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/../include/__support/win32/*.h)
     list(APPEND LIBCXX_HEADERS ${LIBCXX_WIN32_HEADERS})
   endif()
   # Force them all into the headers dir on MSVC, otherwise they end up at
   # project scope because they don't have extensions.
   if (MSVC_IDE)
     source_group("Header Files" FILES ${LIBCXX_HEADERS})
   endif()
 endif()
if(WIN32 AND NOT MINGW AND NOT "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL "Windows")
     # Since we most likely do not have a mt.exe replacement, disable the
     # manifest bundling.  This allows a normal cmake invocation to pass which
     # will attempt to use the manifest tool to generate the bundled manifest
     set_target_properties(cxx_shared PROPERTIES
                           APPEND_STRING PROPERTY LINK_FLAGS " /MANIFEST:NO")
   endif()
 endif()
 
-include(HandleExceptionLibrary)
-
 set(CMAKE_STATIC_LIBRARY_PREFIX "lib")
 
 # Build the static library.
 if (LIBCXX_ENABLE_STATIC)
   add_library(cxx_static STATIC ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_include_directories(cxx_static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
   target_link_libraries(cxx_static PUBLIC cxx-headers
                                    PRIVATE ${LIBCXX_LIBRARIES}
                                    PRIVATE libcxx-abi-static)
   set_target_properties(cxx_static
     PROPERTIES
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "${LIBCXX_STATIC_OUTPUT_NAME}"
   )
   cxx_add_common_build_flags(cxx_static)
   cxx_set_common_defines(cxx_static)
 
   if (LIBCXX_HERMETIC_STATIC_LIBRARY)
     # If the hermetic library doesn't define the operator new/delete functions
     # then its code shouldn't declare them with hidden visibility.  They might
     # actually be provided by a shared library at link time.
     if (LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS)
       append_flags_if_supported(CXX_STATIC_LIBRARY_FLAGS -fvisibility-global-new-delete-hidden)
     endif()

libcxxabi/src/CMakeLists.txt

 # Get sources
 set(LIBCXXABI_SOURCES
   # C++ABI files
   cxa_aux_runtime.cpp
   cxa_default_handlers.cpp
   cxa_demangle.cpp
   cxa_exception_storage.cpp
   cxa_guard.cpp
   cxa_handlers.cpp
   cxa_vector.cpp
   cxa_virtual.cpp
   # C++ STL files
+  exceptions.cpp
   stdlib_exception.cpp
   stdlib_stdexcept.cpp
   stdlib_typeinfo.cpp
   # Internal files
   abort_message.cpp
   fallback_malloc.cpp
   private_typeinfo.cpp
 )

libcxxabi/src/exceptions.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 
 #include <any>
 #include <memory>
 #include <optional>
 #include <variant>
 
-#if defined(__MVS__)
 namespace std {
-#else
-_LIBCPP_BEGIN_NAMESPACE_STD
-#endif
 bad_weak_ptr::~bad_weak_ptr() noexcept {}
-
 const char* bad_weak_ptr::what() const noexcept { return "bad_weak_ptr"; }
-#if defined(__MVS__)
-}
-#else
-_LIBCPP_END_NAMESPACE_STD
-#endif
 
-namespace std {
 const char* bad_any_cast::what() const noexcept { return "bad any cast"; }
 
 bad_optional_access::~bad_optional_access() noexcept = default;
 const char* bad_optional_access::what() const noexcept { return "bad_optional_access"; }
 
 const char* bad_variant_access::what() const noexcept { return "bad_variant_access"; }
 } // namespace std