[libcxx-commits] [PATCH] D99913: [SystemZ][z/OS] Modify cxxabi to be compatible with existing z/OS runtime

[Muiez Ahmed via Phabricator via libcxx-commits]

muiez added inline comments.


================
Comment at: libcxxabi/src/CMakeLists.txt:52
+  list(APPEND LIBCXXABI_HEADERS
+    ${LLVM_EXTERNAL_UNWIND_SOURCE_DIR}/unwind.h
+  )
----------------
muiez wrote:
> ldionne wrote:
> > 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?
> 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.
> 
>>! 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. 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99913/new/

https://reviews.llvm.org/D99913