[SystemZ][z/OS] Modify cxxabi to be compatible with existing z/OS runtime

[muiez]

Summary

This patch is to enable exception handling on the z/OS platform that is compatible with the existing z/OS runtime. No functionality of libcxxabi has been changed for other platforms. With this patch the hope is we can add z/OS as a platform to perform testing on any C++ ABI changes.

There is a primary difference for the z/OS implementation. On z/OS the thrown object is added to a linked list of caught and uncaught exceptions. The unwinder uses the top one as the current exception it is trying to find the landing pad for. We have to pop the top exception after we get it’s landing pad for our unwinder to correctly get any subsequent rethrows or nested exception calls.

[Jonathan.Crowther]

Ping

[abhina.sreeskantharajan]

libcxxabi/src/cxa_personality.cpp

888

minor nit: This can be changed to #elif

[zibi]

libcxxabi/include/zos/unwind.h

2

Please add the copyright statement.

49

I see you use context as parameter name for some functions and ctx` for others. Perhaps you can make this consistent for readability.

54

[muiez]

libcxxabi/src/cxa_personality.cpp

1087

minor nit: would be nice to fix the spacing

[Jonathan.Crowther]

Addressed comments

[MaskRay]

libcxxabi/include/zos/unwind.h

50

The LLVM coding standard doesn't add a space after the function name.

libcxxabi/src/cxa_personality.cpp

605

This is a regression on other platforms.
results.languageSpecificData is now nullptr.

[MaskRay]

libcxxabi/include/zos/unwind.h

64

If you are going to implement these, I highly recommend that you split the implementation from the main Itanium implementaion.

An ARM contributor has signed up the work to extract the ARM EH impl since it is so different.
I am feeling similarly for z/OS. Coupling z/OS specific logic makes the generic Itanium implementation much more difficult.

[Jonathan.Crowther]

If you are going to implement these, I highly recommend that you split the implementation from the main Itanium implementaion.

An ARM contributor has signed up the work to extract the ARM EH impl since it is so different.
I am feeling similarly for z/OS. Coupling z/OS specific logic makes the generic Itanium implementation much more difficult.

We would like as best as possible to use the generic itanium implementation. To do so we will be revising our z/OS runtime to match the interfaces of Itanium . Once this is done I will be able to remove a large portion of the z/OS specific changes. I'll leave this patch inactive while we handle that on our end and I will ping once I have made the revisions.

libcxxabi/include/zos/unwind.h

64

If you are going to implement these, I highly recommend that you split the implementation from the main Itanium implementaion.

An ARM contributor has signed up the work to extract the ARM EH impl since it is so different.
I am feeling similarly for z/OS. Coupling z/OS specific logic makes the generic Itanium implementation much more difficult.

[muiez]

Address comments. Use common API by getting LSDA from the unwinder using _Unwind_GetLanguageSpecificdata.

[muiez]

Removed whitespace

[muiez]

Use external unwind.h header.

[muiez]

Ping :)

[SeanP]

LGTM

[muiez]

kickoff CI

[muiez]

Ping @libc++abi

[muiez]

Ping, all comments addressed and green CI.

[ldionne]

libcxxabi/src/CMakeLists.txt

51

Where is LLVM_EXTERNAL_UNWIND_SOURCE_DIR defined? I just want to make sure we're not introducing yet another way of building libc++/libc++abi using external magically provided headers, since that roughly means it's going to be not supported officially from the start.

Is there a reason why we aren't using LLVM's libunwind in the tree as we normally do?

[SeanP]

Can you update the summary to remove the part about including the unwind header.

[muiez]

definition of LLVM_EXTERNAL_UNWIND_SOURCE_DIR added

[muiez]

libcxxabi/src/CMakeLists.txt

51

I updated the revision to add the definition of LLVM_EXTERNAL_UNWIND_SOURCE_DIR to a zOS.cmake file.

On z/OS, the unwind library is unique from the one provided in LLVM. It needs to interact with undocumented parts of LE. For the purposes of libc++abi, the unwind library is an external library that is not part of the standard C runtime. The build needs to add the -I to find the header and -L/-l to find the library.

A subsequent PR would be made to support the unwinder as an external project.

[zibi]

libcxx/cmake/caches/zOS.cmake

1 ↗

(On Diff #389610)

nit: I would rename this file to be zos.cmake to match what we have for libcxx downstream yet to be upstreamed.

[muiez]

renamed zos cache file name

[muiez]

formalize LLVM_EXTERNAL_UNWIND_SOURCE_DIR definition in libcxxabi/CMakeLists.txt

[muiez]

Added comment

[ldionne]

Is there a reason why we're not adding _Unwind_PopException to LLVM's libunwind and then using that libunwind instead?

If we really want to allow using an arbitrary "external" unwinding library, then I would like to see the following path explored instead: Define an interface target in CMake that represents the unwinding library, and have libcxx and libcxxabi use it. That interface target could represent either the LLVM libunwind or something selected by the user. In your case, you'd use your own interface target (and the definition of that wouldn't even have to be upstreamed). Instead of adding yet another ad-hoc way to tweak the build, it would add the general capability to pick a different unwinder, and would clean up the existing mess around LIBCXXABI_USE_LLVM_UNWINDER & friends by generalizing the setting.

[ldionne]

We can chat on Discord if you want more info on what I'm thinking about.

[MaskRay]

(In a trip; having limited Internet connection)
Drive-by comment:
AIUI _Unwind_ is the https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html namespace but also used by some GCC extensions.
Defining a _Unwind_ identifier may have potential for a conflict with libgcc if they aren't informed.

[ldionne]

In D99913#3174140, @MaskRay wrote:

(In a trip; having limited Internet connection)
Drive-by comment:
AIUI _Unwind_ is the https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html namespace but also used by some GCC extensions.
Defining a _Unwind_ identifier may have potential for a conflict with libgcc if they aren't informed.

I guess they could add _UnwindZOS_PopException when building LLVM's libunwind on z/OS.

Nonetheless, I think the "building with an external libunwind" approach is fine, but we should do it properly, not with ad-hoc include_directories and introducing a new CMake variable that only works on z/OS.

[SeanP]

In D99913#3173537, @ldionne wrote:

We can chat on Discord if you want more info on what I'm thinking about.

Thanks Louis. We'll join you on Discord shortly. We just need to some internal approvals. Could you point us (either here or elsewhere) to what we need to do in Discord to join the chats, that would be great.

[ldionne]

In D99913#3174157, @SeanP wrote:

In D99913#3173537, @ldionne wrote:

We can chat on Discord if you want more info on what I'm thinking about.

Thanks Louis. We'll join you on Discord shortly. We just need to some internal approvals. Could you point us (either here or elsewhere) to what we need to do in Discord to join the chats, that would be great.

Sure thing! This is the link: https://discord.gg/jzUbyP26tQ (documented on https://libcxx.llvm.org/Contributing.html). You'll want to join the #libcxx channel. It'll be easier to have design discussions there.

[muiez]

libcxxabi/src/CMakeLists.txt

51

In D99913#3173529, @ldionne wrote:

Is there a reason why we're not adding _Unwind_PopException to LLVM's libunwind and then using that libunwind instead?

If we really want to allow using an arbitrary "external" unwinding library, then I would like to see the following path explored instead: Define an interface target in CMake that represents the unwinding library, and have libcxx and libcxxabi use it. That interface target could represent either the LLVM libunwind or something selected by the user. In your case, you'd use your own interface target (and the definition of that wouldn't even have to be upstreamed). Instead of adding yet another ad-hoc way to tweak the build, it would add the general capability to pick a different unwinder, and would clean up the existing mess around LIBCXXABI_USE_LLVM_UNWINDER & friends by generalizing the setting.

@ldionne I'm not sure how we can get away with not including the unwind.h header from the external unwinder when building libc++abi. We'd get the following error, since the declaration is in the z/OS unwind.h:

libcxxabi/src/cxa_exception.cpp:444:5: error: use of undeclared identifier '_UnwindZOS_PopException'; did you mean '_Unwind_Exception'?
    _UnwindZOS_PopException();
    ^

We do have a similar approach (in CMake, as mentioned above) to build the external unwinder and use it within libc++abi, which will be a separate patch. For now, we need to include the external unwind.h in order to build the abi library.

[muiez]

Ping :)

[muiez]

@libc++abi Can I get a review please? Thanks

[muiez]

Removed logic for using an external unwinder. The following patch will add support for building/linking an external unwinder in a non-ad hoc way: https://reviews.llvm.org/D128348.

[muiez]

Ping :) @ldionne @libcxxabi

[ldionne]

LGTM, this isn't much now. IIUC this will require D128348 to do anything useful.