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

Repository: rG LLVM Github Monorepo

Event Timeline

fanbo-meng created this revision.Jan 31 2022, 8:48 AM

Herald added subscribers: arichardson, mgorny. · View Herald TranscriptJan 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

Herald added a subscriber: libcxx-commits. · 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.

Harbormaster completed remote builds in B146656: Diff 404554.Jan 31 2022, 11:17 AM

muiez mentioned this in D111323: [SystemZ][z/OS] ASCII/EBCDIC support for libc++.Feb 3 2022, 9:57 AM

LGTM

ping :)

Update zos cmake cache

LGTM

Harbormaster completed remote builds in B149728: Diff 408915.Feb 15 2022, 10:41 AM

rebase to latest main

Harbormaster completed remote builds in B149983: Diff 409276.Feb 16 2022, 12:11 PM

run clang-format

Harbormaster completed remote builds in B150063: Diff 409390.Feb 16 2022, 6:12 PM

formatting __config; rename new sources files to have unique names

Harbormaster completed remote builds in B150224: Diff 409631.Feb 17 2022, 8:06 AM

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.

Addressing comments

fanbo-meng marked 3 inline comments as done.Mar 9 2022, 6:56 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.

Harbormaster completed remote builds in B153348: Diff 414086.Mar 9 2022, 8:30 AM

ping :) @libcxx

D128084 [libc++] Add CI job testing on FreeBSD

·Reviewers: infrandomness, emaste, dim, Restricted Project, Restricted Project

Mon, Jun 27, 5:49 AM

Author: ldionne

D128348 [SystemZ][z/OS] Add support for an external unwinder

·Reviewers: ldionne, zibi, fanbo-meng, Restricted Project, Restricted Project

Fri, Jun 24, 8:20 AM

Author: muiez

D118179 [SystemZ][z/OS] Localize runtime error messages in libc++

·Reviewers: zibi, SeanP, ldionne, Restricted Project, fanbo-meng, Restricted Project

Thu, Jun 23, 3:09 PM

Author: muiez

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

Details

Diff Detail

Event Timeline

No Actions

Revision Contents
Changeset List

Diff 414086

libcxx/cmake/caches/zos.cmake

libcxx/include/__config

libcxx/include/__memory/shared_ptr.h

libcxx/src/CMakeLists.txt

libcxx/src/any.cpp

libcxx/src/exceptions/exception_any.cpp

libcxx/src/exceptions/exception_memory.cpp

libcxx/src/exceptions/exception_optional.cpp

libcxx/src/exceptions/exception_variant.cpp

libcxx/src/memory.cpp

libcxx/src/optional.cpp

libcxx/src/variant.cpp

[SystemZ][z/OS] Build several exception derived classes as a separate libraryNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

No Actions

Revision ContentsChangeset List

Diff 414086

libcxx/cmake/caches/zos.cmake

libcxx/include/__config

libcxx/include/__memory/shared_ptr.h

libcxx/src/CMakeLists.txt

libcxx/src/any.cpp

libcxx/src/exceptions/exception_any.cpp

libcxx/src/exceptions/exception_memory.cpp

libcxx/src/exceptions/exception_optional.cpp

libcxx/src/exceptions/exception_variant.cpp

libcxx/src/memory.cpp

libcxx/src/optional.cpp

libcxx/src/variant.cpp

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

Revision Contents
Changeset List