Page MenuHomePhabricator

[SystemZ][z/OS] Build several exception derived classes as a separate library
Needs ReviewPublic

Authored by fanbo-meng on Jan 31 2022, 8:48 AM.

Details

Reviewers
SeanP
zibi
muiez
ldionne
Group Reviewers
Restricted Project
Summary

These derived classes (std::__1::bad_weak_ptr, std::bad_variant_access, std::bad_any_cast, std::bad_optional_access) of std::exception should not be built twice when we build the library in ASCII & EBCDIC modes for z/OS (https://reviews.llvm.org/D118503). Therefore for z/OS we move the definitions into new files and build them as a separate library in addition to the ASCII/EBCDIC versions of the library. This does not affect other platforms as we would add these files to LIBCXX_SOURCES in CMakeLists.txt and build them normally.

To remove the ABI namespace in std::__1::bad_weak_ptr for z/OS but not affecting other platforms, we define two new namespace macros _LIBCPP_BEGIN_NAMESPACE_EXCEPTION and _LIBCPP_END_NAMESPACE_EXCEPTION and use these as its namespace. This namespace would be defined to std:: for z/OS and _LIBCPP_BEGIN_NAMESPACE_STD/_LIBCPP_END_NAMESPACE_STD for other platforms.

Diff Detail

Event Timeline

fanbo-meng created this revision.Jan 31 2022, 8:48 AM
fanbo-meng requested review of this revision.Jan 31 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2022, 8:48 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
fanbo-meng edited the summary of this revision. (Show Details)Jan 31 2022, 8:50 AM
fanbo-meng added reviewers: SeanP, zibi, muiez.
fanbo-meng edited the summary of this revision. (Show Details)
fanbo-meng added a reviewer: ldionne.
muiez accepted this revision.Feb 4 2022, 10:13 AM

LGTM

Update zos cmake cache

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

LGTM

rebase to latest main

run clang-format

formatting __config; rename new sources files to have unique names

zibi accepted this revision.Feb 22 2022, 4:47 PM

LGTM

libcxx/cmake/caches/zos.cmake
10

This suggestion reflects the recent removal of standalone build.

ldionne requested changes to this revision.Mar 2 2022, 9:55 AM
ldionne added inline comments.
libcxx/include/__config
801–804

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
296

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_any.cpp
10

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

12

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

libcxx/src/exceptions/exception_variant.cpp
15

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.

This revision now requires changes to proceed.Mar 2 2022, 9:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 9:55 AM
fanbo-meng added inline comments.Mar 9 2022, 6:34 AM
libcxx/include/__config
801–804

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
296

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 added inline comments.Mar 9 2022, 6:47 AM
libcxx/src/exceptions/exception_any.cpp
12

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 updated this revision to Diff 414086.Mar 9 2022, 6:55 AM

Addressing comments

fanbo-meng marked 3 inline comments as done.Mar 9 2022, 6:56 AM
zibi accepted this revision.Mar 9 2022, 7:47 AM

LGTM

zibi added inline comments.Mar 9 2022, 7:58 AM
libcxx/src/CMakeLists.txt
312

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 added a comment.EditedMar 24 2022, 12:21 PM

ping :) @libcxx