[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.

[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/variant.cpp

0

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.