This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Reviewers
SeanP
zibi
muiez
ldionne
philnik
EricWF
Group Reviewers
Restricted Project
Restricted Project
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.

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
9 ↗(On Diff #409631)

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/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 added inline comments.Mar 2 2022, 9:55 AM
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.

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 ↗(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 added inline comments.Mar 9 2022, 6:47 AM
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 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
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 added a comment.EditedMar 24 2022, 12:21 PM

ping :) @libcxx

philnik added inline comments.
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 updated this revision to Diff 465484.Oct 5 2022, 11:32 AM
  • Rebase and move recipe to build c++_exception to separate cmake.
zibi updated this revision to Diff 465493.Oct 5 2022, 11:49 AM
  • config.py should not be part of this change
zibi updated this revision to Diff 465538.Oct 5 2022, 1:36 PM
  • formating changes and make set_target_properties for cxx_exception be consistent with cxx_shared
zibi updated this revision to Diff 465556.Oct 5 2022, 2:22 PM
more formating
zibi updated this revision to Diff 465563.Oct 5 2022, 2:53 PM
  • switch to use override in place of virtual
fanbo-meng added inline comments.Oct 6 2022, 6:00 AM
libcxx/include/__memory/shared_ptr.h
50

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

zibi updated this revision to Diff 465716.Oct 6 2022, 6:35 AM
  • use system notation for #include directive
zibi added a comment.Oct 6 2022, 6:42 AM

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

philnik accepted this revision as: philnik.Oct 6 2022, 7:06 AM

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 updated this revision to Diff 466066.Oct 7 2022, 6:52 AM
  • consolidate multiple exceptions files into a single file
zibi added a comment.Oct 11 2022, 7:01 AM

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

zibi added a comment.Oct 21 2022, 6:22 AM

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

EricWF requested changes to this revision.Nov 7 2022, 7:28 AM
EricWF added a subscriber: 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?

This revision now requires changes to proceed.Nov 7 2022, 7:28 AM
zibi updated this revision to Diff 473721.Nov 7 2022, 9:48 AM
  • Remove _LIBCPP_BEGIN_NAMESPACE_EXCEPTION and _LIBCPP_END_NAMESPACE_EXCEPTION
zibi added a comment.Nov 7 2022, 9:48 AM

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 updated this revision to Diff 473743.Nov 7 2022, 10:30 AM
  • correct typo

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 added a comment.Nov 15 2022, 5:46 PM

@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 requested changes to this revision.Nov 21 2022, 1:44 PM

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.

This revision now requires changes to proceed.Nov 21 2022, 1:44 PM
zibi retitled this revision from [SystemZ][z/OS] Build several exception derived classes as a separate library to [SystemZ][z/OS] Move several exception derived classes c++abi library.Nov 28 2022, 12:02 PM
zibi edited the summary of this revision. (Show Details)
zibi updated this revision to Diff 478318.Nov 28 2022, 12:04 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 12:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zibi retitled this revision from [SystemZ][z/OS] Move several exception derived classes c++abi library to [SystemZ][z/OS] Move several exception derived classes to c++abi library.Nov 28 2022, 1:05 PM
zibi updated this revision to Diff 478347.Nov 28 2022, 1:07 PM
  • trying to fix CI.
zibi updated this revision to Diff 478423.Nov 28 2022, 5:16 PM
  • fix formatting
zibi added a comment.Nov 29 2022, 2:16 PM

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

fanbo-meng abandoned this revision.Nov 30 2022, 5:07 AM