This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by muiez on Apr 5 2021, 7:50 PM.

Details

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.

Diff Detail

Event Timeline

Jonathan.Crowther requested review of this revision.Apr 5 2021, 7:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2021, 7:50 PM
Jonathan.Crowther added reviewers: abhina.sreeskantharajan, anirudhp. Jonathan.Crowther removed 1 blocking reviewer(s): Restricted Project.May 5 2021, 6:21 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMay 5 2021, 6:21 AM
libcxxabi/src/cxa_personality.cpp
918

minor nit: This can be changed to #elif

zibi added inline comments.May 5 2021, 6:50 AM
libcxxabi/include/zos/unwind.h
1 ↗(On Diff #335382)

Please add the copyright statement.

48 ↗(On Diff #335382)

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

53 ↗(On Diff #335382)
muiez added inline comments.May 5 2021, 7:52 AM
libcxxabi/src/cxa_personality.cpp
1126

minor nit: would be nice to fix the spacing

Jonathan.Crowther removed a reviewer: Restricted Project.

Addressed comments

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 12 2021, 11:37 AM
MaskRay added inline comments.May 12 2021, 1:15 PM
libcxxabi/include/zos/unwind.h
50 ↗(On Diff #344885)

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

libcxxabi/src/cxa_personality.cpp
627

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

MaskRay added inline comments.May 12 2021, 1:18 PM
libcxxabi/include/zos/unwind.h
64 ↗(On Diff #344885)

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 marked 5 inline comments as done.May 19 2021, 11:22 AM

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

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 commandeered this revision.Nov 2 2021, 6:55 AM
muiez edited reviewers, added: Jonathan.Crowther; removed: muiez.
muiez updated this revision to Diff 384077.Nov 2 2021, 7:02 AM
muiez edited the summary of this revision. (Show Details)
muiez removed a reviewer: Jonathan.Crowther.

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

muiez updated this revision to Diff 384085.Nov 2 2021, 7:06 AM

Removed whitespace

muiez removed a reviewer: anirudhp.Nov 2 2021, 7:06 AM
muiez marked an inline comment as done.Nov 2 2021, 7:13 AM
muiez updated this revision to Diff 384099.Nov 2 2021, 7:28 AM
muiez marked an inline comment as done.Nov 2 2021, 7:38 AM
muiez updated this revision to Diff 384207.Nov 2 2021, 1:32 PM
muiez edited the summary of this revision. (Show Details)

Use external unwind.h header.

muiez updated this revision to Diff 386514.Nov 11 2021, 7:50 AM
SeanP accepted this revision.Nov 11 2021, 8:58 AM

LGTM

muiez updated this revision to Diff 386539.Nov 11 2021, 9:11 AM

kickoff CI

muiez added a comment.Nov 17 2021, 6:28 AM

Ping @libc++abi

muiez added a comment.Nov 24 2021, 8:42 AM

Ping, all comments addressed and green CI.

ldionne added inline comments.Nov 24 2021, 8:47 AM
libcxxabi/src/CMakeLists.txt
52 ↗(On Diff #386539)

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 added a comment.Nov 24 2021, 9:44 AM

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

muiez edited the summary of this revision. (Show Details)Nov 24 2021, 10:07 AM
muiez updated this revision to Diff 389610.Nov 24 2021, 2:04 PM

definition of LLVM_EXTERNAL_UNWIND_SOURCE_DIR added

muiez marked an inline comment as done.Nov 24 2021, 2:08 PM
muiez added inline comments.
libcxxabi/src/CMakeLists.txt
52 ↗(On Diff #386539)

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 added inline comments.Nov 25 2021, 6:38 AM
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 updated this revision to Diff 389781.Nov 25 2021, 7:13 AM
muiez marked an inline comment as done.

renamed zos cache file name

muiez updated this revision to Diff 390781.Nov 30 2021, 12:24 PM

formalize LLVM_EXTERNAL_UNWIND_SOURCE_DIR definition in libcxxabi/CMakeLists.txt

muiez updated this revision to Diff 391657.Dec 3 2021, 9:21 AM

Added comment

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.

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

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

(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 added a comment.Dec 6 2021, 10:44 AM

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.

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 added inline comments.Jan 4 2022, 7:00 AM
libcxxabi/src/CMakeLists.txt
52 ↗(On Diff #386539)

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.

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

muiez updated this revision to Diff 437697.EditedJun 16 2022, 1:57 PM
muiez edited the summary of this revision. (Show Details)
muiez set the repository for this revision to rG LLVM Github Monorepo.

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.

Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 1:57 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
muiez added a comment.Jun 24 2022, 7:19 AM

Ping :) @ldionne @libcxxabi

ldionne accepted this revision.Jun 27 2022, 3:07 PM

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

This revision is now accepted and ready to land.Jun 27 2022, 3:07 PM
MaskRay accepted this revision.Jun 27 2022, 5:55 PM